feat(language-service): provide diagnostic for invalid templateUrls (#32586)
`templateUrls` that do not point to actual files are now diagnosed as such by the Language Service. Support for `styleUrls` will come in a next PR. This introduces a utility method `getPropertyValueOfType` that scans TypeScript ASTs until a property assignment whose initializer of a certain type is found. This PR also notices a couple of things that could be improved in the language-service implementation, such as enumerating directive properties and unifying common logic, that will be fixed in future PRs. Part of #32564. PR Close #32586
This commit is contained in:
		
							parent
							
								
									88c28ce208
								
							
						
					
					
						commit
						adeee0fa7f
					
				| @ -8,11 +8,13 @@ | |||||||
| 
 | 
 | ||||||
| import {NgAnalyzedModules} from '@angular/compiler'; | import {NgAnalyzedModules} from '@angular/compiler'; | ||||||
| import {getTemplateExpressionDiagnostics} from '@angular/compiler-cli/src/language_services'; | import {getTemplateExpressionDiagnostics} from '@angular/compiler-cli/src/language_services'; | ||||||
|  | import * as path from 'path'; | ||||||
| import * as ts from 'typescript'; | import * as ts from 'typescript'; | ||||||
| 
 | 
 | ||||||
| import {AstResult} from './common'; | import {AstResult} from './common'; | ||||||
| import * as ng from './types'; | import * as ng from './types'; | ||||||
| import {offsetSpan, spanOf} from './utils'; | import {TypeScriptServiceHost} from './typescript_host'; | ||||||
|  | import {findPropertyValueOfType, findTightestNode, offsetSpan, spanOf} from './utils'; | ||||||
| 
 | 
 | ||||||
| /** | /** | ||||||
|  * Return diagnostic information for the parsed AST of the template. |  * Return diagnostic information for the parsed AST of the template. | ||||||
| @ -53,8 +55,24 @@ function missingDirective(name: string, isComponent: boolean) { | |||||||
|       'available inside a template. Consider adding it to a NgModule declaration.'; |       'available inside a template. Consider adding it to a NgModule declaration.'; | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  | /** | ||||||
|  |  * Logs an error for an impossible state with a certain message. | ||||||
|  |  */ | ||||||
|  | function logImpossibleState(message: string) { | ||||||
|  |   console.error(`Impossible state: ${message}`); | ||||||
|  | } | ||||||
|  | 
 | ||||||
|  | /** | ||||||
|  |  * Performs a variety diagnostics on directive declarations. | ||||||
|  |  * | ||||||
|  |  * @param declarations Angular directive declarations | ||||||
|  |  * @param modules NgModules in the project | ||||||
|  |  * @param host TypeScript service host used to perform TypeScript queries | ||||||
|  |  * @return diagnosed errors, if any | ||||||
|  |  */ | ||||||
| export function getDeclarationDiagnostics( | export function getDeclarationDiagnostics( | ||||||
|     declarations: ng.Declaration[], modules: NgAnalyzedModules): ng.Diagnostic[] { |     declarations: ng.Declaration[], modules: NgAnalyzedModules, | ||||||
|  |     host: Readonly<TypeScriptServiceHost>): ng.Diagnostic[] { | ||||||
|   const directives = new Set<ng.StaticSymbol>(); |   const directives = new Set<ng.StaticSymbol>(); | ||||||
|   for (const ngModule of modules.ngModules) { |   for (const ngModule of modules.ngModules) { | ||||||
|     for (const directive of ngModule.declaredDirectives) { |     for (const directive of ngModule.declaredDirectives) { | ||||||
| @ -66,6 +84,20 @@ export function getDeclarationDiagnostics( | |||||||
| 
 | 
 | ||||||
|   for (const declaration of declarations) { |   for (const declaration of declarations) { | ||||||
|     const {errors, metadata, type, declarationSpan} = declaration; |     const {errors, metadata, type, declarationSpan} = declaration; | ||||||
|  | 
 | ||||||
|  |     const sf = host.getSourceFile(type.filePath); | ||||||
|  |     if (!sf) { | ||||||
|  |       logImpossibleState(`directive ${type.name} exists but has no source file`); | ||||||
|  |       return []; | ||||||
|  |     } | ||||||
|  |     // TypeScript identifier of the directive declaration annotation (e.g. "Component" or
 | ||||||
|  |     // "Directive") on a directive class.
 | ||||||
|  |     const directiveIdentifier = findTightestNode(sf, declarationSpan.start); | ||||||
|  |     if (!directiveIdentifier) { | ||||||
|  |       logImpossibleState(`directive ${type.name} exists but has no identifier`); | ||||||
|  |       return []; | ||||||
|  |     } | ||||||
|  | 
 | ||||||
|     for (const error of errors) { |     for (const error of errors) { | ||||||
|       results.push({ |       results.push({ | ||||||
|         kind: ng.DiagnosticKind.Error, |         kind: ng.DiagnosticKind.Error, | ||||||
| @ -91,12 +123,28 @@ export function getDeclarationDiagnostics( | |||||||
|           message: `Component '${type.name}' must have a template or templateUrl`, |           message: `Component '${type.name}' must have a template or templateUrl`, | ||||||
|           span: declarationSpan, |           span: declarationSpan, | ||||||
|         }); |         }); | ||||||
|       } else if (template && templateUrl) { |       } else if (templateUrl) { | ||||||
|         results.push({ |         if (template) { | ||||||
|           kind: ng.DiagnosticKind.Error, |           results.push({ | ||||||
|           message: `Component '${type.name}' must not have both template and templateUrl`, |             kind: ng.DiagnosticKind.Error, | ||||||
|           span: declarationSpan, |             message: `Component '${type.name}' must not have both template and templateUrl`, | ||||||
|         }); |             span: declarationSpan, | ||||||
|  |           }); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|  |         // Find templateUrl value from the directive call expression, which is the parent of the
 | ||||||
|  |         // directive identifier.
 | ||||||
|  |         //
 | ||||||
|  |         // TODO: We should create an enum of the various properties a directive can have to use
 | ||||||
|  |         // instead of string literals. We can then perform a mass migration of all literal usages.
 | ||||||
|  |         const templateUrlNode = findPropertyValueOfType( | ||||||
|  |             directiveIdentifier.parent, 'templateUrl', ts.isStringLiteralLike); | ||||||
|  |         if (!templateUrlNode) { | ||||||
|  |           logImpossibleState(`templateUrl ${templateUrl} exists but its TypeScript node doesn't`); | ||||||
|  |           return []; | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|  |         results.push(...validateUrls([templateUrlNode], host.tsLsHost)); | ||||||
|       } |       } | ||||||
|     } else if (!directives.has(declaration.type)) { |     } else if (!directives.has(declaration.type)) { | ||||||
|       results.push({ |       results.push({ | ||||||
| @ -110,6 +158,39 @@ export function getDeclarationDiagnostics( | |||||||
|   return results; |   return results; | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  | /** | ||||||
|  |  * Checks that URLs on a directive point to a valid file. | ||||||
|  |  * Note that this diagnostic check may require a filesystem hit, and thus may be slower than other | ||||||
|  |  * checks. | ||||||
|  |  * | ||||||
|  |  * @param urls urls to check for validity | ||||||
|  |  * @param tsLsHost TS LS host used for querying filesystem information | ||||||
|  |  * @return diagnosed url errors, if any | ||||||
|  |  */ | ||||||
|  | function validateUrls( | ||||||
|  |     urls: ts.StringLiteralLike[], tsLsHost: Readonly<ts.LanguageServiceHost>): ng.Diagnostic[] { | ||||||
|  |   if (!tsLsHost.fileExists) { | ||||||
|  |     return []; | ||||||
|  |   } | ||||||
|  | 
 | ||||||
|  |   const allErrors: ng.Diagnostic[] = []; | ||||||
|  |   // TODO(ayazhafiz): most of this logic can be unified with the logic in
 | ||||||
|  |   // definitions.ts#getUrlFromProperty. Create a utility function to be used by both.
 | ||||||
|  |   for (const urlNode of urls) { | ||||||
|  |     const curPath = urlNode.getSourceFile().fileName; | ||||||
|  |     const url = path.join(path.dirname(curPath), urlNode.text); | ||||||
|  |     if (tsLsHost.fileExists(url)) continue; | ||||||
|  | 
 | ||||||
|  |     allErrors.push({ | ||||||
|  |       kind: ng.DiagnosticKind.Error, | ||||||
|  |       message: `URL does not point to a valid file`, | ||||||
|  |       // Exclude opening and closing quotes in the url span.
 | ||||||
|  |       span: {start: urlNode.getStart() + 1, end: urlNode.end - 1}, | ||||||
|  |     }); | ||||||
|  |   } | ||||||
|  |   return allErrors; | ||||||
|  | } | ||||||
|  | 
 | ||||||
| /** | /** | ||||||
|  * Return a recursive data structure that chains diagnostic messages. |  * Return a recursive data structure that chains diagnostic messages. | ||||||
|  * @param chain |  * @param chain | ||||||
|  | |||||||
| @ -37,6 +37,7 @@ class LanguageServiceImpl implements LanguageService { | |||||||
|     const analyzedModules = this.host.getAnalyzedModules();  // same role as 'synchronizeHostData'
 |     const analyzedModules = this.host.getAnalyzedModules();  // same role as 'synchronizeHostData'
 | ||||||
|     const results: Diagnostic[] = []; |     const results: Diagnostic[] = []; | ||||||
|     const templates = this.host.getTemplates(fileName); |     const templates = this.host.getTemplates(fileName); | ||||||
|  | 
 | ||||||
|     for (const template of templates) { |     for (const template of templates) { | ||||||
|       const astOrDiagnostic = this.host.getTemplateAst(template); |       const astOrDiagnostic = this.host.getTemplateAst(template); | ||||||
|       if (isAstResult(astOrDiagnostic)) { |       if (isAstResult(astOrDiagnostic)) { | ||||||
| @ -45,10 +46,12 @@ class LanguageServiceImpl implements LanguageService { | |||||||
|         results.push(astOrDiagnostic); |         results.push(astOrDiagnostic); | ||||||
|       } |       } | ||||||
|     } |     } | ||||||
|  | 
 | ||||||
|     const declarations = this.host.getDeclarations(fileName); |     const declarations = this.host.getDeclarations(fileName); | ||||||
|     if (declarations && declarations.length) { |     if (declarations && declarations.length) { | ||||||
|       results.push(...getDeclarationDiagnostics(declarations, analyzedModules)); |       results.push(...getDeclarationDiagnostics(declarations, analyzedModules, this.host)); | ||||||
|     } |     } | ||||||
|  | 
 | ||||||
|     const sourceFile = fileName.endsWith('.ts') ? this.host.getSourceFile(fileName) : undefined; |     const sourceFile = fileName.endsWith('.ts') ? this.host.getSourceFile(fileName) : undefined; | ||||||
|     return uniqueBySpan(results).map(d => ngDiagnosticToTsDiagnostic(d, sourceFile)); |     return uniqueBySpan(results).map(d => ngDiagnosticToTsDiagnostic(d, sourceFile)); | ||||||
|   } |   } | ||||||
|  | |||||||
| @ -214,3 +214,21 @@ export function getDirectiveClassLike(node: ts.Node): DirectiveClassLike|undefin | |||||||
|     } |     } | ||||||
|   } |   } | ||||||
| } | } | ||||||
|  | 
 | ||||||
|  | /** | ||||||
|  |  * Finds the value of a property assignment that is nested in a TypeScript node and is of a certain | ||||||
|  |  * type T. | ||||||
|  |  * | ||||||
|  |  * @param startNode node to start searching for nested property assignment from | ||||||
|  |  * @param propName property assignment name | ||||||
|  |  * @param predicate function to verify that a node is of type T. | ||||||
|  |  * @return node property assignment value of type T, or undefined if none is found | ||||||
|  |  */ | ||||||
|  | export function findPropertyValueOfType<T extends ts.Node>( | ||||||
|  |     startNode: ts.Node, propName: string, predicate: (node: ts.Node) => node is T): T|undefined { | ||||||
|  |   if (ts.isPropertyAssignment(startNode) && startNode.name.getText() === propName) { | ||||||
|  |     const {initializer} = startNode; | ||||||
|  |     if (predicate(initializer)) return initializer; | ||||||
|  |   } | ||||||
|  |   return startNode.forEachChild(c => findPropertyValueOfType(c, propName, predicate)); | ||||||
|  | } | ||||||
|  | |||||||
| @ -475,6 +475,40 @@ describe('diagnostics', () => { | |||||||
|             `Module '"../node_modules/@angular/core/core"' has no exported member 'OpaqueToken'.`); |             `Module '"../node_modules/@angular/core/core"' has no exported member 'OpaqueToken'.`); | ||||||
|   }); |   }); | ||||||
| 
 | 
 | ||||||
|  |   describe('URL diagnostics', () => { | ||||||
|  |     it('should report errors for invalid templateUrls', () => { | ||||||
|  |       const fileName = mockHost.addCode(` | ||||||
|  | 	@Component({ | ||||||
|  |           templateUrl: '«notAFile»', | ||||||
|  |         }) | ||||||
|  |         export class MyComponent {}`);
 | ||||||
|  | 
 | ||||||
|  |       const marker = mockHost.getReferenceMarkerFor(fileName, 'notAFile'); | ||||||
|  | 
 | ||||||
|  |       const diagnostics = ngLS.getDiagnostics(fileName) !; | ||||||
|  |       const urlDiagnostic = | ||||||
|  |           diagnostics.find(d => d.messageText === 'URL does not point to a valid file'); | ||||||
|  |       expect(urlDiagnostic).toBeDefined(); | ||||||
|  | 
 | ||||||
|  |       const {start, length} = urlDiagnostic !; | ||||||
|  |       expect(start).toBe(marker.start); | ||||||
|  |       expect(length).toBe(marker.length); | ||||||
|  |     }); | ||||||
|  | 
 | ||||||
|  |     it('should not report errors for valid templateUrls', () => { | ||||||
|  |       const fileName = mockHost.addCode(` | ||||||
|  | 	@Component({ | ||||||
|  |           templateUrl: './test.ng', | ||||||
|  | 	}) | ||||||
|  | 	export class MyComponent {}`);
 | ||||||
|  | 
 | ||||||
|  |       const diagnostics = ngLS.getDiagnostics(fileName) !; | ||||||
|  |       const urlDiagnostic = | ||||||
|  |           diagnostics.find(d => d.messageText === 'URL does not point to a valid file'); | ||||||
|  |       expect(urlDiagnostic).toBeUndefined(); | ||||||
|  |     }); | ||||||
|  |   }); | ||||||
|  | 
 | ||||||
|   // https://github.com/angular/vscode-ng-language-service/issues/235
 |   // https://github.com/angular/vscode-ng-language-service/issues/235
 | ||||||
|   // There is no easy fix for this issue currently due to the way template
 |   // There is no easy fix for this issue currently due to the way template
 | ||||||
|   // tokenization is done. In the example below, the whole string
 |   // tokenization is done. In the example below, the whole string
 | ||||||
|  | |||||||
		Loading…
	
	
			
			x
			
			
		
	
		Reference in New Issue
	
	Block a user