From 81241af7ac1d320ad17feba1b69fd34f04ac278a Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Thu, 6 Feb 2020 14:49:49 -0800 Subject: [PATCH] fix(language-service): Suggest ? and ! operator on nullable receiver (#35200) Under strict mode, the language service fails to typecheck nullable symbols that have already been verified to be non-null. This generates incorrect (false positive) and confusing diagnostics for users. To work around this issue in the short term, this commit changes the diagnostic message from an error to a suggestion, and prompts users to use the safe navigation operator (?) or non-null assertion operator (!). For example, instead of ```typescript {{ optional && optional.toString() }} ``` the following is cleaner: ```typescript {{ optional?.toString() }} {{ optional!.toString() }} ``` Note that with this change, users who legitimately make a typo in their code will no longer see an error. I think this is acceptable, since false positive is worse than false negative. However, if users follow the suggestion, add ? or ! to their code, then the error will be surfaced. This seems a reasonable trade-off. References: 1. Safe navigation operator (?) https://angular.io/guide/template-syntax#the-safe-navigation-operator----and-null-property-paths 2. Non-null assertion operator (!) https://angular.io/guide/template-syntax#the-non-null-assertion-operator--- PR closes https://github.com/angular/angular/pull/35070 PR closes https://github.com/angular/vscode-ng-language-service/issues/589 PR Close #35200 --- .../language-service/src/expression_type.ts | 56 +++++++--------- .../language-service/test/diagnostics_spec.ts | 67 ++++++++++++++++++- .../test/expression_diagnostics_spec.ts | 7 +- .../test/project/app/parsing-cases.ts | 1 + 4 files changed, 98 insertions(+), 33 deletions(-) diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index 3bdeec102b..911ed80fad 100644 --- a/packages/language-service/src/expression_type.ts +++ b/packages/language-service/src/expression_type.ts @@ -366,22 +366,19 @@ export class AstType implements AstVisitor { if (this.isAny(receiverType)) { return this.anyType; } - - // The type of a method is the selected methods result type. - const method = receiverType.members().get(ast.name); - if (!method) { - this.reportDiagnostic(`Unknown method '${ast.name}'`, ast); - return this.anyType; - } - if (!method.type) { + const methodType = this.resolvePropertyRead(receiverType, ast); + if (!methodType) { this.reportDiagnostic(`Could not find a type for '${ast.name}'`, ast); return this.anyType; } - if (!method.type.callable) { + if (this.isAny(methodType)) { + return this.anyType; + } + if (!methodType.callable) { this.reportDiagnostic(`Member '${ast.name}' is not callable`, ast); return this.anyType; } - const signature = method.type.selectSignature(ast.args.map(arg => this.getType(arg))); + const signature = methodType.selectSignature(ast.args.map(arg => this.getType(arg))); if (!signature) { this.reportDiagnostic(`Unable to resolve signature for call of method ${ast.name}`, ast); return this.anyType; @@ -393,34 +390,33 @@ export class AstType implements AstVisitor { if (this.isAny(receiverType)) { return this.anyType; } - // The type of a property read is the seelcted member's type. const member = receiverType.members().get(ast.name); if (!member) { - let receiverInfo = receiverType.name; - if (receiverInfo == '$implicit') { - receiverInfo = - 'The component declaration, template variable declarations, and element references do'; - } else if (receiverType.nullable) { - return this.reportDiagnostic(`The expression might be null`, ast.receiver); + if (receiverType.name === '$implicit') { + this.reportDiagnostic( + `Identifier '${ast.name}' is not defined. ` + + `The component declaration, template variable declarations, and element references do not contain such a member`, + ast); + } else if (receiverType.nullable && ast.receiver instanceof PropertyRead) { + const receiver = ast.receiver.name; + this.reportDiagnostic( + `'${receiver}' is possibly undefined. Consider using the safe navigation operator (${receiver}?.${ast.name}) ` + + `or non-null assertion operator (${receiver}!.${ast.name}).`, + ast, ts.DiagnosticCategory.Suggestion); } else { - receiverInfo = `'${receiverInfo}' does`; + this.reportDiagnostic( + `Identifier '${ast.name}' is not defined. '${receiverType.name}' does not contain such a member`, + ast); } - this.reportDiagnostic( - `Identifier '${ast.name}' is not defined. ${receiverInfo} not contain such a member`, - ast); return this.anyType; } if (!member.public) { - let receiverInfo = receiverType.name; - if (receiverInfo == '$implicit') { - receiverInfo = 'the component'; - } else { - receiverInfo = `'${receiverInfo}'`; - } this.reportDiagnostic( - `Identifier '${ast.name}' refers to a private member of ${receiverInfo}`, ast, - ts.DiagnosticCategory.Warning); + `Identifier '${ast.name}' refers to a private member of ${receiverType.name === '$implicit' ? 'the component' : ` + '${receiverType.name}' + `}`, + ast, ts.DiagnosticCategory.Warning); } return member.type; } @@ -430,7 +426,7 @@ export class AstType implements AstVisitor { } private isAny(symbol: Symbol): boolean { - return !symbol || this.query.getTypeKind(symbol) == BuiltinType.Any || + return !symbol || this.query.getTypeKind(symbol) === BuiltinType.Any || (!!symbol.type && this.isAny(symbol.type)); } } diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 3cccaa1f2b..9597b0dd62 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -339,7 +339,9 @@ describe('diagnostics', () => { const diagnostics = ngLS.getSemanticDiagnostics(TEST_TEMPLATE) !; expect(diagnostics.length).toBe(1); const {messageText, start, length} = diagnostics[0]; - expect(messageText).toBe(`Unknown method 'notSubstring'`); + expect(messageText) + .toBe( + `Identifier 'notSubstring' is not defined. 'string' does not contain such a member`); expect(start).toBe(content.indexOf('$event')); expect(length).toBe('$event.notSubstring()'.length); }); @@ -885,4 +887,67 @@ describe('diagnostics', () => { const ngDiags = ngLS.getSemanticDiagnostics(fileName); expect(ngDiags).toEqual([]); }); + + it('should suggest ? or ! operator if method receiver is nullable', () => { + const content = mockHost.override(TEST_TEMPLATE, `{{optional && optional.toLowerCase()}}`); + const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(ngDiags.length).toBe(1); + const {start, length, messageText, category} = ngDiags[0]; + expect(messageText) + .toBe( + `'optional' is possibly undefined. ` + + `Consider using the safe navigation operator (optional?.toLowerCase) ` + + `or non-null assertion operator (optional!.toLowerCase).`); + expect(category).toBe(ts.DiagnosticCategory.Suggestion); + expect(content.substring(start !, start ! + length !)).toBe('optional.toLowerCase()'); + + }); + + it('should suggest ? or ! operator if property receiver is nullable', () => { + const content = mockHost.override(TEST_TEMPLATE, `{{optional && optional.length}}`); + const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(ngDiags.length).toBe(1); + const {start, length, messageText, category} = ngDiags[0]; + expect(messageText) + .toBe( + `'optional' is possibly undefined. ` + + `Consider using the safe navigation operator (optional?.length) ` + + `or non-null assertion operator (optional!.length).`); + expect(category).toBe(ts.DiagnosticCategory.Suggestion); + expect(content.substring(start !, start ! + length !)).toBe('optional.length'); + }); + + it('should report error if method is not found on non-nullable receiver', () => { + const expressions = [ + 'optional?.someMethod()', + 'optional!.someMethod()', + ]; + for (const expression of expressions) { + const content = mockHost.override(TEST_TEMPLATE, `{{${expression}}}`); + const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(ngDiags.length).toBe(1); + const {start, length, messageText, category} = ngDiags[0]; + expect(messageText) + .toBe(`Identifier 'someMethod' is not defined. 'string' does not contain such a member`); + expect(category).toBe(ts.DiagnosticCategory.Error); + expect(content.substring(start !, start ! + length !)).toBe(expression); + } + }); + + it('should report error if property is not found on non-nullable receiver', () => { + const expressions = [ + 'optional?.someProp', + 'optional!.someProp', + ]; + for (const expression of expressions) { + const content = mockHost.override(TEST_TEMPLATE, `{{${expression}}}`); + const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(ngDiags.length).toBe(1); + const {start, length, messageText, category} = ngDiags[0]; + expect(messageText) + .toBe(`Identifier 'someProp' is not defined. 'string' does not contain such a member`); + expect(category).toBe(ts.DiagnosticCategory.Error); + expect(content.substring(start !, start ! + length !)).toBe(expression); + } + }); }); diff --git a/packages/language-service/test/expression_diagnostics_spec.ts b/packages/language-service/test/expression_diagnostics_spec.ts index 4cd3c15a13..d2fb2c7db9 100644 --- a/packages/language-service/test/expression_diagnostics_spec.ts +++ b/packages/language-service/test/expression_diagnostics_spec.ts @@ -144,7 +144,9 @@ describe('expression diagnostics', () => { `, 'Identifier \'nume\' is not defined')); it('should reject access to potentially undefined field', - () => reject(`
{{maybe_person.name.first}}`, 'The expression might be null')); + () => reject( + `
{{maybe_person.name.first}}`, + `'maybe_person' is possibly undefined. Consider using the safe navigation operator (maybe_person?.name) or non-null assertion operator (maybe_person!.name).`)); it('should accept a safe accss to an undefined field', () => accept(`
{{maybe_person?.name.first}}
`)); it('should accept a type assert to an undefined field', @@ -183,7 +185,8 @@ describe('expression diagnostics', () => { () => accept('
{{person.name.first}}
')); it('should reject a misspelled event handler', () => reject( - '
{{person.name.first}}
', 'Unknown method \'clack\'')); + '
{{person.name.first}}
', + `Identifier 'clack' is not defined. The component declaration, template variable declarations, and element references do not contain such a member`)); it('should reject an uncalled event handler', () => reject( '
{{person.name.first}}
', 'Unexpected callable expression')); diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 90c90a9eaf..7e62dbf6c1 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -154,6 +154,7 @@ export class TemplateReference { heroesByName: {[name: string]: Hero} = {}; primitiveIndexType: {[name: string]: string} = {}; anyValue: any; + optional?: string; myClick(event: any) {} }