Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning diagnostic when using V2 api #33

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/pyright-internal/src/analyzer/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ import {
VariableDeclaration,
} from './declaration';
import { ImplicitImport, ImportResult, ImportType } from './importResult';
import { device, maybeAddMicrobitVersionWarningBinderWrapper } from './microbitUtils';
import * as ParseTreeUtils from './parseTreeUtils';
import { ParseTreeWalker } from './parseTreeWalker';
import { NameBindingType, Scope, ScopeType } from './scope';
Expand Down Expand Up @@ -386,6 +387,19 @@ export class Binder extends ParseTreeWalker {
}
}

// Add warning diagnostic for V2 micro:bit modules.
maybeAddMicrobitVersionWarningBinderWrapper(importResult.importName, () =>
this._addDiagnostic(
this._fileInfo.diagnosticRuleSet.reportMicrobitV2ApiUse,
DiagnosticRule.reportMicrobitV2ApiUse,
Localizer.Diagnostic.microbitV2ModuleUse().format({
moduleName: importResult.importName,
device,
}),
node
)
);
Comment on lines +391 to +401
Copy link
Author

@microbit-robert microbit-robert May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is setup with a callback so we can track micro:bit specific diagnostics in one file, microbitUtils.ts. Useful if we only want to warn on the first use of a V2 API call in the future. We can't directly pass in this_.addDiagnostic as this causes errors.


return true;
}

Expand Down
156 changes: 156 additions & 0 deletions packages/pyright-internal/src/analyzer/microbitUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
import { DiagnosticLevel } from '../common/configOptions';
import { Diagnostic } from '../common/diagnostic';
import { DiagnosticRule } from '../common/diagnosticRules';
import { Localizer } from '../localization/localize';
import { NameNode, ParseNode } from '../parser/parseNodes';
import { AnalyzerFileInfo } from './analyzerFileInfo';
import * as AnalyzerNodeInfo from './analyzerNodeInfo';
import { isClass, isFunction, isModule, isOverloadedFunction, Type } from './types';

type AddDiagnostic = (
diagLevel: DiagnosticLevel,
rule: string,
message: string,
node: ParseNode
) => Diagnostic | undefined;

export const device = 'micro:bit V1';

function getNames(type: Type) {
if (isFunction(type) || isClass(type)) {
return {
moduleName: type.details.moduleName,
name: type.details.name,
};
}
if (isOverloadedFunction(type)) {
return {
moduleName: type.overloads[0].details.moduleName,
name: type.overloads[0].details.name,
};
}
return {
moduleName: '',
name: '',
};
}

function usesMicrobitV2Api(moduleName: string, name?: string) {
return (
['log', 'microbit.microphone', 'microbit.speaker', 'power'].includes(moduleName) ||
(moduleName === 'microbit' &&
['run_every', 'set_volume', 'Sound', 'SoundEvent', 'pin_logo', 'pin_speaker'].includes(name ?? '')) ||
(moduleName === 'microbit.audio' && name === 'SoundEffect') ||
(moduleName === 'neopixel' && ['fill', 'write'].includes(name ?? ''))
);
}

function getFileInfo(node: ParseNode): AnalyzerFileInfo {
return AnalyzerNodeInfo.getFileInfo(node);
}

function getDiagLevel(fileInfo: AnalyzerFileInfo): DiagnosticLevel {
return fileInfo.diagnosticRuleSet.reportMicrobitV2ApiUse;
}

function addModuleVersionWarning(addDiagnostic: AddDiagnostic, moduleName: string, node: ParseNode) {
const fileInfo = getFileInfo(node);
addDiagnostic(
getDiagLevel(fileInfo),
DiagnosticRule.reportMicrobitV2ApiUse,
Localizer.Diagnostic.microbitV2ModuleUse().format({
moduleName: moduleName,
device,
}),
node
);
}

function addModuleMemberVersionWarning(
addDiagnostic: AddDiagnostic,
name: string,
moduleName: string,
node: ParseNode
) {
const fileInfo = getFileInfo(node);
addDiagnostic(
getDiagLevel(fileInfo),
DiagnosticRule.reportMicrobitV2ApiUse,
Localizer.Diagnostic.microbitV2ModuleMemberUse().format({
name,
moduleName,
device,
}),
node
);
}

function addClassMethodVersionWarning(
addDiagnostic: AddDiagnostic,
methodName: string,
className: string,
node: ParseNode
) {
const fileInfo = getFileInfo(node);
addDiagnostic(
getDiagLevel(fileInfo),
DiagnosticRule.reportMicrobitV2ApiUse,
Localizer.Diagnostic.microbitV2ClassMethodUse().format({
methodName,
className,
device,
}),
node
);
}

export function maybeAddMicrobitVersionWarning(
type: Type,
node: ParseNode,
addDiagnostic: AddDiagnostic,
memberName?: string
) {
if (isModule(type)) {
if (usesMicrobitV2Api(type.moduleName, memberName)) {
addModuleVersionWarning(addDiagnostic, type.moduleName, node);
}
}

const moduleName = getNames(type).moduleName;
let name = getNames(type).name;
if (!moduleName && !name) {
return;
}

// Special case pin_logo and pin_speaker.
if (name === 'MicroBitAnalogDigitalPin' && (node as NameNode).value === 'pin_speaker') {
name = 'pin_speaker';
}
if (name === 'MicroBitTouchPin' && (node as NameNode).value === 'pin_logo') {
name = 'pin_logo';
}

if (usesMicrobitV2Api(moduleName, name)) {
if (isFunction(type) && type.boundToType) {
const className = type.boundToType?.details.name;
addClassMethodVersionWarning(addDiagnostic, name, className, node);
return;
}

if (isClass(type) && type.includeSubclasses) {
// Bail. Example, we only want to warn for microbit.Sound once and not again for microbit.Sound.GIGGLE.
return;
}

// The type must be a function, overloaded function or class at this point.
addModuleMemberVersionWarning(addDiagnostic, name, moduleName, node);
}
}

// Required as passing in this._addDiagnostic results in errors.
// See binder.ts for use.
export function maybeAddMicrobitVersionWarningBinderWrapper(moduleName: string, callback: () => void) {
if (usesMicrobitV2Api(moduleName)) {
callback();
}
}
18 changes: 18 additions & 0 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ import {
isPossibleTypeAliasDeclaration,
} from './declarationUtils';
import { applyFunctionTransform } from './functionTransform';
import { maybeAddMicrobitVersionWarning } from './microbitUtils';
import { createNamedTupleType } from './namedTuples';
import * as ParseTreeUtils from './parseTreeUtils';
import { assignTypeToPatternTargets, narrowTypeBasedOnPattern } from './patternMatching';
Expand Down Expand Up @@ -3854,6 +3855,10 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
type = validateTypeVarUsage(node, type, flags);
}

// Maybe add warning diagnostic for V2 micro:bit module members
// (functions) where these are imported.
maybeAddMicrobitVersionWarning(type, node, addDiagnostic);

return { type, node, isIncomplete };
}

Expand Down Expand Up @@ -4526,6 +4531,12 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
isAsymmetricDescriptor = true;
}
}

if (type) {
// Maybe add warning diagnostic for V2 micro:bit class methods.
maybeAddMicrobitVersionWarning(type, node, addDiagnostic);
}

break;
}

Expand Down Expand Up @@ -4614,6 +4625,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
type = evaluatorOptions.evaluateUnknownImportsAsAny ? AnyType.create() : UnknownType.create();
}
}

// Maybe add warning diagnostic for V2 micro:bit modules or module members.
maybeAddMicrobitVersionWarning(type, node, addDiagnostic, memberName);
break;
}

Expand Down Expand Up @@ -16203,6 +16217,10 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
}
}

// Maybe add warning diagnostic for V2 micro:bit module
// members for `from module import member`.
maybeAddMicrobitVersionWarning(symbolType, node.name, addDiagnostic);

assignTypeToNameNode(aliasNode, symbolType, /* isIncomplete */ false);
writeTypeCache(node, symbolType, EvaluatorFlags.None, /* isIncomplete */ false);
}
Expand Down
7 changes: 7 additions & 0 deletions packages/pyright-internal/src/common/configOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,9 @@ export interface DiagnosticRuleSet {
// Report cases where the a "match" statement is not exhaustive in
// covering all possible cases.
reportMatchNotExhaustive: DiagnosticLevel;

// Report micro:bit V2 API use.
reportMicrobitV2ApiUse: DiagnosticLevel;
}

export function cloneDiagnosticRuleSet(diagSettings: DiagnosticRuleSet): DiagnosticRuleSet {
Expand Down Expand Up @@ -374,6 +377,7 @@ export function getDiagLevelDiagnosticRules() {
DiagnosticRule.reportUnusedCoroutine,
DiagnosticRule.reportUnnecessaryTypeIgnoreComment,
DiagnosticRule.reportMatchNotExhaustive,
DiagnosticRule.reportMicrobitV2ApiUse,
];
}

Expand Down Expand Up @@ -453,6 +457,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet {
reportUnusedCoroutine: 'none',
reportUnnecessaryTypeIgnoreComment: 'none',
reportMatchNotExhaustive: 'none',
reportMicrobitV2ApiUse: 'none',
};

return diagSettings;
Expand Down Expand Up @@ -528,6 +533,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet {
reportUnusedCoroutine: 'error',
reportUnnecessaryTypeIgnoreComment: 'none',
reportMatchNotExhaustive: 'none',
reportMicrobitV2ApiUse: 'warning',
};

return diagSettings;
Expand Down Expand Up @@ -603,6 +609,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet {
reportUnusedCoroutine: 'error',
reportUnnecessaryTypeIgnoreComment: 'none',
reportMatchNotExhaustive: 'error',
reportMicrobitV2ApiUse: 'warning',
};

return diagSettings;
Expand Down
2 changes: 2 additions & 0 deletions packages/pyright-internal/src/common/diagnosticRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,6 @@ export enum DiagnosticRule {
reportUnusedCoroutine = 'reportUnusedCoroutine',
reportUnnecessaryTypeIgnoreComment = 'reportUnnecessaryTypeIgnoreComment',
reportMatchNotExhaustive = 'reportMatchNotExhaustive',

reportMicrobitV2ApiUse = 'reportMicrobitV2ApiUse',
}
12 changes: 12 additions & 0 deletions packages/pyright-internal/src/localization/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,18 @@ export namespace Localizer {
);
export const methodReturnsNonObject = () =>
new ParameterizedString<{ name: string }>(getRawString('Diagnostic.methodReturnsNonObject'));
export const microbitV2ClassMethodUse = () =>
new ParameterizedString<{ methodName: string; className: string; device: string }>(
getRawString('Diagnostic.microbitV2ClassMethodUse')
);
export const microbitV2ModuleMemberUse = () =>
new ParameterizedString<{ name: string; moduleName: string; device: string }>(
getRawString('Diagnostic.microbitV2ModuleMemberUse')
);
export const microbitV2ModuleUse = () =>
new ParameterizedString<{ moduleName: string; device: string }>(
getRawString('Diagnostic.microbitV2ModuleUse')
);
export const missingProtocolMembers = () => getRawString('Diagnostic.missingProtocolMembers');
export const missingSuperCall = () =>
new ParameterizedString<{ methodName: string }>(getRawString('Diagnostic.missingSuperCall'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@
"memberSet": "Cannot assign member \"{name}\" for type \"{type}\"",
"moduleNotCallable": "Module is not callable",
"moduleUnknownMember": "\"{name}\" is not a known member of module \"{module}\"",
"microbitV2ClassMethodUse": "The \"{methodName}\" method on the \"{className}\" class cannot be used with a {device}",
"microbitV2ModuleMemberUse": "\"{name}\" from the \"{moduleName}\" module cannot be used with a {device}",
"microbitV2ModuleUse": "\"{moduleName}\" cannot be used with a {device}",
Comment on lines +61 to +63
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Highlighting for review as these will be tweaked based on prior discussion.

"nonDefaultAfterDefault": "Non-default argument follows default argument",
"noOverload": "Arguments do not match parameter types",
"objectNotCallable": "Object is not callable",
Expand Down