Merge pull request #10 from microbit-foundation/improve-errors

Improve common student error messages Add a new simplified message bundle with simplified text of error message after review with the education team. In simplified mode, types aren't referenced in common messages as students in our scenarios are best focussed on docs and examples to fix their errors. New messages: - booleanIsLowercase special-cases "true" and "false" in name errors as forgetting to uppercase is common - expectedEqualityOperator adds a new case for `if a = b` scenarios Code-level tweaks to existing messages: - add module names to importSymbolUnknown and moduleUnknownMember (potentially upstreamable)
microbit-foundation · May 20, 2022 · 1e46d5f · 1e46d5f
2 parents 510bfee + 80361a3
commit 1e46d5f
Show file tree

Hide file tree

Showing 9 changed files with 158 additions and 25 deletions.
diff --git a/packages/browser-pyright/src/browser-server.ts b/packages/browser-pyright/src/browser-server.ts
@@ -41,6 +41,7 @@ import {
     MarkupKind,
 } from 'vscode-languageserver';
 import { apiDocsRequestType } from 'pyright-internal/apidocsProtocol';
+import { setMessageStyle } from 'pyright-internal/localization/localize';
 
 const maxAnalysisTimeInForeground = { openFilesTimeInMs: 50, noOpenFilesTimeInMs: 200 };
 
@@ -125,6 +126,8 @@ export class PyrightServer extends LanguageServerBase {
             this._initialFiles = files as InitialFiles;
             (this._serverOptions.fileSystem as TestFileSystem).apply(files);
         }
+        // Hack to enable simplified error messages (locale is set in super.initialize).
+        setMessageStyle('simplified');
         return super.initialize(params, supportedCommands, supportedCodeActions);
     }
 
@@ -361,6 +364,10 @@ export class BrowserBackgroundAnalysis extends BackgroundAnalysisBase {
 export class BrowserBackgroundAnalysisRunner extends BackgroundAnalysisRunnerBase {
     constructor(initialData: InitializationData) {
         super(parentPort(), initialData);
+
+        // Hack to enable simplified error messages in the background thread.
+        // Ideally we'd route this via initialData.
+        setMessageStyle('simplified');
     }
     createRealFileSystem(): FileSystem {
         return new TestFileSystem(false, {

diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts
@@ -25,7 +25,7 @@ import { DiagnosticRule } from '../common/diagnosticRules';
 import { convertOffsetsToRange } from '../common/positionUtils';
 import { PythonVersion } from '../common/pythonVersion';
 import { TextRange } from '../common/textRange';
-import { Localizer } from '../localization/localize';
+import { Localizer, optionalAddendum } from '../localization/localize';
 import {
     ArgumentCategory,
     AssignmentNode,
@@ -2311,7 +2311,8 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                 addDiagnostic(
                     AnalyzerNodeInfo.getFileInfo(errorNode).diagnosticRuleSet.reportGeneralTypeIssues,
                     DiagnosticRule.reportGeneralTypeIssues,
-                    Localizer.Diagnostic.typeNotIterable().format({ type: printType(subtype) }) + diag.getString(),
+                    Localizer.Diagnostic.typeNotIterable().format({ type: printType(subtype) }) +
+                        optionalAddendum(diag),
                     errorNode
                 );
             }
@@ -3789,8 +3790,20 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                 }
             }
         } else {
+            // Handle the special case of booleans.
+            if (name === 'true' || name === 'false') {
+                const nameSplit = name.split('');
+                nameSplit[0] = nameSplit[0].toUpperCase();
+                const booleanName = nameSplit.join('');
+                addDiagnostic(
+                    fileInfo.diagnosticRuleSet.reportUndefinedVariable,
+                    DiagnosticRule.reportUndefinedVariable,
+                    Localizer.Diagnostic.booleanIsLowerCase().format({ name, booleanName }),
+                    node
+                );
+            }
             // Handle the special case of "reveal_type" and "reveal_locals".
-            if (name !== 'reveal_type' && name !== 'reveal_locals') {
+            else if (name !== 'reveal_type' && name !== 'reveal_locals') {
                 addDiagnostic(
                     fileInfo.diagnosticRuleSet.reportUndefinedVariable,
                     DiagnosticRule.reportUndefinedVariable,
@@ -4588,7 +4601,10 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                             addDiagnostic(
                                 fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
                                 DiagnosticRule.reportGeneralTypeIssues,
-                                Localizer.Diagnostic.moduleUnknownMember().format({ name: memberName }),
+                                Localizer.Diagnostic.moduleUnknownMember().format({
+                                    name: memberName,
+                                    module: baseType.moduleName,
+                                }),
                                 node.memberName
                             );
                         }
@@ -10030,7 +10046,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                 addDiagnostic(
                     fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
                     DiagnosticRule.reportGeneralTypeIssues,
-                    message + diag.getString(),
+                    message + optionalAddendum(diag),
                     argParam.errorNode
                 );
             }
@@ -10872,7 +10888,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                             operator: ParseTreeUtils.printOperator(node.operator),
                             leftType: printType(leftType),
                             rightType: printType(rightType),
-                        }) + diag.getString(),
+                        }) + optionalAddendum(diag),
                         node
                     );
                 }
@@ -11035,7 +11051,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                         operator: ParseTreeUtils.printOperator(node.operator),
                         leftType: printType(leftType),
                         rightType: printType(rightType),
-                    }) + diag.getString(),
+                    }) + optionalAddendum(diag),
                     node
                 );
             }
@@ -16170,7 +16186,10 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
                     addDiagnostic(
                         fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
                         DiagnosticRule.reportGeneralTypeIssues,
-                        Localizer.Diagnostic.importSymbolUnknown().format({ name: node.name.value }),
+                        Localizer.Diagnostic.importSymbolUnknown().format({
+                            name: node.name.value,
+                            moduleName: importInfo.importName,
+                        }),
                         node.name
                     );
                 }

diff --git a/packages/pyright-internal/src/localization/localize.ts b/packages/pyright-internal/src/localization/localize.ts
@@ -18,6 +18,9 @@ import ruStrings = require('./package.nls.ru.json');
 import zhCnStrings = require('./package.nls.zh-cn.json');
 import zhTwStrings = require('./package.nls.zh-tw.json');
 
+import enUsSimplified = require('./simplified.nls.en-us.json');
+import { DiagnosticAddendum } from '../common/diagnostic';
+
 export class ParameterizedString<T extends {}> {
     constructor(private _formatString: string) {}
 
@@ -34,17 +37,31 @@ export class ParameterizedString<T extends {}> {
     }
 }
 
+function mergeStrings(a: any, b: any): any {
+    const keys = new Set([...Object.keys(a), ...Object.keys(b)]);
+    const result: any = {};
+    for (const k of keys) {
+        result[k] = {
+            ...a[k],
+            ...b[k],
+        };
+    }
+    return result;
+}
+
+type MessageStyle = 'default' | 'simplified';
+
+let messageStyle: MessageStyle = 'default';
+
+export function setMessageStyle(style: MessageStyle) {
+    messageStyle = style;
+}
+
+export function optionalAddendum(diag: DiagnosticAddendum) {
+    return messageStyle === 'simplified' ? '' : diag.toString();
+}
+
 const defaultLocale = 'en-us';
-const stringMapsByLocale: Map<string, any> = new Map([
-    ['de', deStrings],
-    ['en-us', enUsStrings],
-    ['es', esStrings],
-    ['fr', frStrings],
-    ['ja', jaStrings],
-    ['ru', ruStrings],
-    ['zh-cn', zhCnStrings],
-    ['zh-tw', zhTwStrings],
-]);
 
 type StringLookupMap = { [key: string]: string | StringLookupMap };
 let localizedStrings: StringLookupMap | undefined = undefined;
@@ -166,7 +183,26 @@ function loadStringsForLocale(locale: string): StringLookupMap {
 }
 
 function loadStringsFromJsonFile(locale: string): StringLookupMap | undefined {
-    return stringMapsByLocale.get(locale);
+    switch (locale) {
+        case 'de':
+            return deStrings;
+        case 'en-us':
+            return messageStyle === 'simplified' ? mergeStrings(enUsStrings, enUsSimplified) : enUsStrings;
+        case 'es':
+            return esStrings;
+        case 'fr':
+            return frStrings;
+        case 'ja':
+            return jaStrings;
+        case 'ru':
+            return ruStrings;
+        case 'zh-cn':
+            return zhCnStrings;
+        case 'zh-tw':
+            return zhTwStrings;
+        default:
+            return undefined;
+    }
 }
 
 export namespace Localizer {
@@ -231,6 +267,10 @@ export namespace Localizer {
             new ParameterizedString<{ type: string; methodName: string; paramName: string }>(
                 getRawString('Diagnostic.bindTypeMismatch')
             );
+        export const booleanIsLowerCase = () =>
+            new ParameterizedString<{ name: string; booleanName: string }>(
+                getRawString('Diagnostic.booleanIsLowerCase')
+            );
         export const breakOutsideLoop = () => getRawString('Diagnostic.breakOutsideLoop');
         export const callableExtraArgs = () => getRawString('Diagnostic.callableExtraArgs');
         export const callableFirstArg = () => getRawString('Diagnostic.callableFirstArg');
@@ -363,6 +403,7 @@ export namespace Localizer {
         export const expectedDecoratorNewline = () => getRawString('Diagnostic.expectedDecoratorNewline');
         export const expectedDelExpr = () => getRawString('Diagnostic.expectedDelExpr');
         export const expectedElse = () => getRawString('Diagnostic.expectedElse');
+        export const expectedEqualityOperator = () => getRawString('Diagnostic.expectedEqualityOperator');
         export const expectedExceptionClass = () => getRawString('Diagnostic.expectedExceptionClass');
         export const expectedExceptionObj = () => getRawString('Diagnostic.expectedExceptionObj');
         export const expectedExpr = () => getRawString('Diagnostic.expectedExpr');
@@ -440,7 +481,9 @@ export namespace Localizer {
         export const importSourceResolveFailure = () =>
             new ParameterizedString<{ importName: string }>(getRawString('Diagnostic.importSourceResolveFailure'));
         export const importSymbolUnknown = () =>
-            new ParameterizedString<{ name: string }>(getRawString('Diagnostic.importSymbolUnknown'));
+            new ParameterizedString<{ name: string; moduleName: string }>(
+                getRawString('Diagnostic.importSymbolUnknown')
+            );
         export const incompatibleMethodOverride = () =>
             new ParameterizedString<{ name: string; className: string }>(
                 getRawString('Diagnostic.incompatibleMethodOverride')
@@ -518,7 +561,7 @@ export namespace Localizer {
         export const moduleAsType = () => getRawString('Diagnostic.moduleAsType');
         export const moduleNotCallable = () => getRawString('Diagnostic.moduleNotCallable');
         export const moduleUnknownMember = () =>
-            new ParameterizedString<{ name: string }>(getRawString('Diagnostic.moduleUnknownMember'));
+            new ParameterizedString<{ name: string; module: string }>(getRawString('Diagnostic.moduleUnknownMember'));
         export const namedExceptAfterCatchAll = () => getRawString('Diagnostic.namedExceptAfterCatchAll');
         export const namedParamAfterParamSpecArgs = () =>
             new ParameterizedString<{ name: string }>(getRawString('Diagnostic.namedParamAfterParamSpecArgs'));

diff --git a/packages/pyright-internal/src/localization/package.nls.en-us.json b/packages/pyright-internal/src/localization/package.nls.en-us.json
@@ -35,6 +35,7 @@
         "baseClassMethodTypeIncompatible": "Base classes for class \"{classType}\" define method \"{name}\" in incompatible way",
         "baseClassUnknown": "Base class type is unknown, obscuring type of derived class",
         "bindTypeMismatch": "Could not bind method \"{methodName}\" because \"{type}\" is not assignable to parameter \"{paramName}\"",
+        "booleanIsLowerCase": "\"{name}\" is not defined, did you mean \"{booleanName}\"?",
         "breakOutsideLoop": "\"break\" can be used only within a loop",
         "callableExtraArgs": "Expected only two type arguments to \"Callable\"",
         "callableFirstArg": "Expected parameter type list or \"...\"",
@@ -125,6 +126,7 @@
         "expectedDecoratorNewline": "Expected new line at end of decorator",
         "expectedDelExpr": "Expected expression after \"del\"",
         "expectedElse": "Expected \"else\"",
+        "expectedEqualityOperator": "Expected equality operator, did you mean \"==\"?",
         "expectedExceptionClass": "Invalid exception class or object",
         "expectedExceptionObj": "Expected exception object, exception class or None",
         "expectedExpr": "Expected expression",
@@ -185,7 +187,7 @@
         "importDepthExceeded": "Import chain depth exceeded {depth}",
         "importResolveFailure": "Import \"{importName}\" could not be resolved",
         "importSourceResolveFailure": "Import \"{importName}\" could not be resolved from source",
-        "importSymbolUnknown": "\"{name}\" is unknown import symbol",
+        "importSymbolUnknown": "\"{name}\" is unknown import symbol in module \"{moduleName}\"",
         "incompatibleMethodOverride": "Method \"{name}\" overrides class \"{className}\" in an incompatible manner",
         "inconsistentIndent": "Unindent amount does not match previous indent",
         "initMustReturnNone": "Return type of \"__init__\" must be None",
@@ -232,7 +234,7 @@
         "missingSuperCall": "Method \"{methodName}\" does not call the method of the same name in parent class",
         "moduleAsType": "Module cannot be used as a type",
         "moduleNotCallable": "Module is not callable",
-        "moduleUnknownMember": "\"{name}\" is not a known member of module",
+        "moduleUnknownMember": "\"{name}\" is not a known member of module \"{module}\"",
         "namedExceptAfterCatchAll": "A named except clause cannot appear after catch-all except clause",
         "namedParamAfterParamSpecArgs": "Keyword parameter \"{name}\" cannot appear in signature after ParamSpec args parameter",
         "namedTupleEmptyName": "Names within a named tuple cannot be empty",

diff --git a/packages/pyright-internal/src/localization/simplified.nls.en-us.json b/packages/pyright-internal/src/localization/simplified.nls.en-us.json
@@ -0,0 +1,37 @@
+{
+    "Diagnostic": {
+        "argAssignmentParam": "Argument does not match parameter type for parameter \"{paramName}\"",
+        "argAssignmentParamFunction": "Argument does not match parameter type for parameter \"{paramName}\"",
+        "argMissingForParam": "Argument missing for parameter {name}",
+        "argMissingForParams": "Arguments missing for parameters {names}",
+        "argMorePositionalExpectedCount": "Expected {expected} more positional arguments",
+        "booleanIsLowerCase": "\"{name}\" is not defined, did you mean \"{booleanName}\"?",
+        "breakOutsideLoop": "\"break\" can be used only within a loop",
+        "continueOutsideLoop": "\"continue\" can be used only within a loop",
+        "expectedAssignRightHandExpr": "Expected expression to the right of \"=\"",
+        "expectedCloseBrace": "Missing closing bracket \"}\"",
+        "expectedCloseBracket": "Missing closing bracket \"]\"",
+        "expectedCloseParen": "Missing closing bracket \")\"",
+        "expectedColon": "Missing colon \":\"",
+        "expectedEqualityOperator": "Expected equality operator, did you mean \"==\"?",
+        "expectedExpr": "Missing expression",
+        "expectedFunctionName": "Missing function name after \"def\"",
+        "expectedIndentedBlock": "Indentation missing",
+        "expectedNewlineOrSemicolon": "Unexpected extra content\nStatements must be one per line",
+        "importResolveFailure": "Module \"{importName}\" could not be found",
+        "importSymbolUnknown": "\"{name}\" not found in module \"{moduleName}\"",
+        "inconsistentIndent": "Indentation does not match the previous line",
+        "moduleUnknownMember": "\"{name}\" is not a known member of module \"{module}\"",
+        "stringUnterminated": "String is not closed — missing quotation mark",
+        "typeNotIterable": "Type is not iterable",
+        "typeNotSupportBinaryOperator": "Operator \"{operator}\" not supported for this combination of types",
+        "typeNotSupportBinaryOperatorBidirectional": "Operator \"{operator}\" not supported for this combination of types",
+        "unaccessedClass": "Class \"{name}\" is unused",
+        "unaccessedFunction": "Function \"{name}\" is unused",
+        "unaccessedImport": "Import \"{name}\" is unused",
+        "unaccessedSymbol": "\"{name}\" is unused",
+        "unaccessedVariable": "Variable \"{name}\" is unused",
+        "unexpectedIndent": "Unexpected indentation",
+        "unreachableCode": "Code is unreachable\nThe logic of your program means this code will never run"
+    }
+}
diff --git a/packages/pyright-internal/src/parser/parser.ts b/packages/pyright-internal/src/parser/parser.ts
@@ -1209,8 +1209,13 @@ export class Parser {
         const suite = SuiteNode.create(nextToken);
 
         if (!this._consumeTokenIfType(TokenType.Colon)) {
-            this._addError(Localizer.Diagnostic.expectedColon(), nextToken);
-
+            if (nextToken.type === TokenType.Operator) {
+                if (this._peekOperatorType() === OperatorType.Assign) {
+                    this._addError(Localizer.Diagnostic.expectedEqualityOperator(), nextToken);
+                }
+            } else {
+                this._addError(Localizer.Diagnostic.expectedColon(), nextToken);
+            }
             // Try to perform parse recovery by consuming tokens.
             if (this._consumeTokensUntilType([TokenType.NewLine, TokenType.Colon])) {
                 if (this._peekTokenType() === TokenType.Colon) {

diff --git a/packages/pyright-internal/src/tests/parser.test.ts b/packages/pyright-internal/src/tests/parser.test.ts
@@ -57,6 +57,20 @@ test('SuiteExpectedColon3', () => {
     assert.strictEqual(diagSink.getErrors().length, 1);
 });
 
+test('SuiteExpectedColon4', () => {
+    const diagSink = new DiagnosticSink();
+    TestUtils.parseSampleFile('suiteExpectedColon4.py', diagSink);
+    assert.strictEqual(diagSink.getErrors().length, 1);
+    assert.strictEqual(diagSink.getErrors()[0].message, 'Expected equality operator, did you mean "=="?');
+});
+
+test('SuiteExpectedColon5', () => {
+    const diagSink = new DiagnosticSink();
+    TestUtils.parseSampleFile('suiteExpectedColon5.py', diagSink);
+    assert.strictEqual(diagSink.getErrors().length, 1);
+    assert.strictEqual(diagSink.getErrors()[0].message, 'Expected ":"');
+});
+
 test('ExpressionWrappedInParens', () => {
     const diagSink = new DiagnosticSink();
     const parseResults = TestUtils.parseText('(str)', diagSink);

diff --git a/packages/pyright-internal/src/tests/samples/suiteExpectedColon4.py b/packages/pyright-internal/src/tests/samples/suiteExpectedColon4.py
@@ -0,0 +1,3 @@
+a = 1
+if a = 1:
+  pass
diff --git a/packages/pyright-internal/src/tests/samples/suiteExpectedColon5.py b/packages/pyright-internal/src/tests/samples/suiteExpectedColon5.py
@@ -0,0 +1,3 @@
+a = 1
+if a == 1
+  pass