From 6fe6bda2cbc7dcca7f56c768903ed99607497ace Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 31 Mar 2021 12:16:26 -0700 Subject: [PATCH 1/2] fix(compiler-cli): Allow analysis to continue with invalid style url Currently, we throw a FatalDiagnosticError when we fail to load a resource (`templateUrl` or `styleUrl`) at various stages in the compiler. This prevents analysis of the component from completing. This will result in in users not being able to get any information in the component template when there is a missing `styleUrl`, for example. This commit simply tracks the diagnostic, marks the component as poisoned, and continues merrily along. Environments configured to use poisoned data (like the language service) will then be able to use other information from the analysis. Fixes https://github.com/angular/vscode-ng-language-service/issues/1241 --- .../src/ngtsc/annotations/src/component.ts | 71 ++++++++++++++----- .../ivy/test/quick_info_spec.ts | 34 ++++++++- 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index dd58ad5b56aeb..162f968869f1e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -10,7 +10,7 @@ import {compileComponentFromMetadata, compileDeclareComponentFromMetadata, Const import * as ts from 'typescript'; import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles'; -import {ErrorCode, FatalDiagnosticError, makeRelatedInformation} from '../../diagnostics'; +import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; import {absoluteFrom, relative} from '../../file_system'; import {DefaultImportRecorder, ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; @@ -265,9 +265,15 @@ export class ComponentDecoratorHandler implements const resolveStyleUrl = (styleUrl: string, nodeForError: ts.Node, resourceType: ResourceTypeForDiagnostics): Promise|undefined => { - const resourceUrl = - this._resolveResourceOrThrow(styleUrl, containingFile, nodeForError, resourceType); - return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile}); + try { + const resourceUrl = + this._resolveResourceOrThrow(styleUrl, containingFile, nodeForError, resourceType); + return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile}); + } catch { + // Don't worry about failures to preload. We can handle this problem during analysis by + // producing a diagnostic. + return undefined; + } }; // A Promise that waits for the template and all ed styles within it to be preloaded. @@ -326,6 +332,8 @@ export class ComponentDecoratorHandler implements const containingFile = node.getSourceFile().fileName; this.literalCache.delete(decorator); + let diagnostics: ts.Diagnostic[]|undefined; + let isPoisoned = false; // @Component inherits @Directive, so begin by extracting the @Directive metadata and building // on it. const directiveResult = extractDirectiveMetadata( @@ -411,13 +419,26 @@ export class ComponentDecoratorHandler implements const resourceType = styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ? ResourceTypeForDiagnostics.StylesheetFromDecorator : ResourceTypeForDiagnostics.StylesheetFromTemplate; - const resourceUrl = this._resolveResourceOrThrow( - styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); - const resourceStr = this.resourceLoader.load(resourceUrl); + try { + const resourceUrl = this._resolveResourceOrThrow( + styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); + const resourceStr = this.resourceLoader.load(resourceUrl); - styles.push(resourceStr); - if (this.depTracker !== null) { - this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl)); + styles.push(resourceStr); + + if (this.depTracker !== null) { + this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl)); + } + } catch (e) { + if (e instanceof FatalDiagnosticError) { + if (diagnostics === undefined) { + diagnostics = []; + } + diagnostics.push(makeDiagnostic(e.code, e.node, e.message, e.relatedInformation)); + isPoisoned = true; + } else { + throw e; + } } } @@ -496,8 +517,9 @@ export class ComponentDecoratorHandler implements styles: styleResources, template: templateResource, }, - isPoisoned: false, + isPoisoned, }, + diagnostics, }; if (changeDetection !== null) { output.analysis!.meta.changeDetection = changeDetection; @@ -837,10 +859,15 @@ export class ComponentDecoratorHandler implements styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ? ResourceTypeForDiagnostics.StylesheetFromDecorator : ResourceTypeForDiagnostics.StylesheetFromTemplate; - const resolvedStyleUrl = this._resolveResourceOrThrow( - styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); - const styleText = this.resourceLoader.load(resolvedStyleUrl); - styles.push(styleText); + try { + const resolvedStyleUrl = this._resolveResourceOrThrow( + styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); + const styleText = this.resourceLoader.load(resolvedStyleUrl); + styles.push(styleText); + } catch (e) { + // Resource resolve failures should already be in the diagnostics list from the analyze + // stage. We do not need to do anything with them when updating resources. + } } } if (analysis.inlineStyles !== null) { @@ -978,10 +1005,16 @@ export class ComponentDecoratorHandler implements const styleUrlsExpr = component.get('styleUrls'); if (styleUrlsExpr !== undefined && ts.isArrayLiteralExpression(styleUrlsExpr)) { for (const expression of stringLiteralElements(styleUrlsExpr)) { - const resourceUrl = this._resolveResourceOrThrow( - expression.text, containingFile, expression, - ResourceTypeForDiagnostics.StylesheetFromDecorator); - styles.add({path: absoluteFrom(resourceUrl), expression}); + try { + const resourceUrl = this._resolveResourceOrThrow( + expression.text, containingFile, expression, + ResourceTypeForDiagnostics.StylesheetFromDecorator); + styles.add({path: absoluteFrom(resourceUrl), expression}); + } catch { + // Errors in style resource extraction do not need to be handled here. We will produce + // diagnostics for each one that fails in the analysis, after we evaluate the `styleUrls` + // expression to determine _all_ style resources, not just the string literals. + } } } diff --git a/packages/language-service/ivy/test/quick_info_spec.ts b/packages/language-service/ivy/test/quick_info_spec.ts index 11b610cf171db..9c07fc3457ff4 100644 --- a/packages/language-service/ivy/test/quick_info_spec.ts +++ b/packages/language-service/ivy/test/quick_info_spec.ts @@ -553,8 +553,40 @@ describe('quick info', () => { expectQuickInfo( {templateOverride, expectedSpanText: 'date', expectedDisplayString: '(pipe) DatePipe'}); }); - }); + it('should still get quick info if there is an invalid css resource', () => { + project = env.addProject('test', { + 'app.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'some-cmp', + templateUrl: './app.html', + styleUrls: ['./does_not_exist'], + }) + export class SomeCmp { + myValue!: string; + } + + @NgModule({ + declarations: [SomeCmp], + }) + export class AppModule{ + } + `, + 'app.html': `{{myValue}}`, + }); + const diagnostics = project.getDiagnosticsForFile('app.ts'); + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].messageText) + .toEqual(`Could not find stylesheet file './does_not_exist'.`); + + const template = project.openFile('app.html'); + template.moveCursorToText('{{myVa¦lue}}'); + const quickInfo = template.getQuickInfoAtPosition(); + expect(toText(quickInfo!.displayParts)).toEqual('(property) SomeCmp.myValue: string'); + }); + }); function expectQuickInfo( {templateOverride, expectedSpanText, expectedDisplayString}: From d0b2af3b5520e30b1063b3268aa630fe76bb3014 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 5 Apr 2021 14:24:32 -0700 Subject: [PATCH 2/2] fixup! fix(compiler-cli): Allow analysis to continue with invalid style url --- .../src/ngtsc/annotations/src/component.ts | 180 ++++++++---------- 1 file changed, 78 insertions(+), 102 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 162f968869f1e..5487ac5ee3790 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -262,19 +262,16 @@ export class ComponentDecoratorHandler implements const component = reflectObjectLiteral(meta); const containingFile = node.getSourceFile().fileName; - const resolveStyleUrl = - (styleUrl: string, nodeForError: ts.Node, - resourceType: ResourceTypeForDiagnostics): Promise|undefined => { - try { - const resourceUrl = - this._resolveResourceOrThrow(styleUrl, containingFile, nodeForError, resourceType); - return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile}); - } catch { - // Don't worry about failures to preload. We can handle this problem during analysis by - // producing a diagnostic. - return undefined; - } - }; + const resolveStyleUrl = (styleUrl: string): Promise|undefined => { + try { + const resourceUrl = this.resourceLoader.resolve(styleUrl, containingFile); + return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile}); + } catch { + // Don't worry about failures to preload. We can handle this problem during analysis by + // producing a diagnostic. + return undefined; + } + }; // A Promise that waits for the template and all ed styles within it to be preloaded. const templateAndTemplateStyleResources = @@ -284,12 +281,7 @@ export class ComponentDecoratorHandler implements return undefined; } - const nodeForError = getTemplateDeclarationNodeForError(template.declaration); - return Promise - .all(template.styleUrls.map( - styleUrl => resolveStyleUrl( - styleUrl, nodeForError, - ResourceTypeForDiagnostics.StylesheetFromTemplate))) + return Promise.all(template.styleUrls.map(styleUrl => resolveStyleUrl(styleUrl))) .then(() => undefined); }); @@ -317,10 +309,7 @@ export class ComponentDecoratorHandler implements return Promise .all([ templateAndTemplateStyleResources, inlineStyles, - ...componentStyleUrls.map( - styleUrl => resolveStyleUrl( - styleUrl.url, styleUrl.nodeForError, - ResourceTypeForDiagnostics.StylesheetFromDecorator)) + ...componentStyleUrls.map(styleUrl => resolveStyleUrl(styleUrl.url)) ]) .then(() => undefined); } @@ -416,29 +405,24 @@ export class ComponentDecoratorHandler implements ]; for (const styleUrl of styleUrls) { - const resourceType = styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ? - ResourceTypeForDiagnostics.StylesheetFromDecorator : - ResourceTypeForDiagnostics.StylesheetFromTemplate; try { - const resourceUrl = this._resolveResourceOrThrow( - styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); + const resourceUrl = this.resourceLoader.resolve(styleUrl.url, containingFile); const resourceStr = this.resourceLoader.load(resourceUrl); - styles.push(resourceStr); - if (this.depTracker !== null) { this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl)); } - } catch (e) { - if (e instanceof FatalDiagnosticError) { - if (diagnostics === undefined) { - diagnostics = []; - } - diagnostics.push(makeDiagnostic(e.code, e.node, e.message, e.relatedInformation)); - isPoisoned = true; - } else { - throw e; + } catch { + if (diagnostics === undefined) { + diagnostics = []; } + const resourceType = + styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ? + ResourceTypeForDiagnostics.StylesheetFromDecorator : + ResourceTypeForDiagnostics.StylesheetFromTemplate; + diagnostics.push( + this.makeResourceNotFoundError(styleUrl.url, styleUrl.nodeForError, resourceType) + .toDiagnostic()); } } @@ -855,13 +839,8 @@ export class ComponentDecoratorHandler implements let styles: string[] = []; if (analysis.styleUrls !== null) { for (const styleUrl of analysis.styleUrls) { - const resourceType = - styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ? - ResourceTypeForDiagnostics.StylesheetFromDecorator : - ResourceTypeForDiagnostics.StylesheetFromTemplate; try { - const resolvedStyleUrl = this._resolveResourceOrThrow( - styleUrl.url, containingFile, styleUrl.nodeForError, resourceType); + const resolvedStyleUrl = this.resourceLoader.resolve(styleUrl.url, containingFile); const styleText = this.resourceLoader.load(resolvedStyleUrl); styles.push(styleText); } catch (e) { @@ -1006,9 +985,7 @@ export class ComponentDecoratorHandler implements if (styleUrlsExpr !== undefined && ts.isArrayLiteralExpression(styleUrlsExpr)) { for (const expression of stringLiteralElements(styleUrlsExpr)) { try { - const resourceUrl = this._resolveResourceOrThrow( - expression.text, containingFile, expression, - ResourceTypeForDiagnostics.StylesheetFromDecorator); + const resourceUrl = this.resourceLoader.resolve(expression.text, containingFile); styles.add({path: absoluteFrom(resourceUrl), expression}); } catch { // Errors in style resource extraction do not need to be handled here. We will produce @@ -1039,22 +1016,27 @@ export class ComponentDecoratorHandler implements throw createValueHasWrongTypeError( templateUrlExpr, templateUrl, 'templateUrl must be a string'); } - const resourceUrl = this._resolveResourceOrThrow( - templateUrl, containingFile, templateUrlExpr, ResourceTypeForDiagnostics.Template); - const templatePromise = - this.resourceLoader.preload(resourceUrl, {type: 'template', containingFile}); - - // If the preload worked, then actually load and parse the template, and wait for any style - // URLs to resolve. - if (templatePromise !== undefined) { - return templatePromise.then(() => { - const templateDecl = this.parseTemplateDeclaration(decorator, component, containingFile); - const template = this.extractTemplate(node, templateDecl); - this.preanalyzeTemplateCache.set(node, template); - return template; - }); - } else { - return Promise.resolve(null); + try { + const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile); + const templatePromise = + this.resourceLoader.preload(resourceUrl, {type: 'template', containingFile}); + + // If the preload worked, then actually load and parse the template, and wait for any style + // URLs to resolve. + if (templatePromise !== undefined) { + return templatePromise.then(() => { + const templateDecl = + this.parseTemplateDeclaration(decorator, component, containingFile); + const template = this.extractTemplate(node, templateDecl); + this.preanalyzeTemplateCache.set(node, template); + return template; + }); + } else { + return Promise.resolve(null); + } + } catch (e) { + throw this.makeResourceNotFoundError( + templateUrl, templateUrlExpr, ResourceTypeForDiagnostics.Template); } } else { const templateDecl = this.parseTemplateDeclaration(decorator, component, containingFile); @@ -1219,18 +1201,21 @@ export class ComponentDecoratorHandler implements throw createValueHasWrongTypeError( templateUrlExpr, templateUrl, 'templateUrl must be a string'); } - const resourceUrl = this._resolveResourceOrThrow( - templateUrl, containingFile, templateUrlExpr, ResourceTypeForDiagnostics.Template); - - return { - isInline: false, - interpolationConfig, - preserveWhitespaces, - templateUrl, - templateUrlExpression: templateUrlExpr, - resolvedTemplateUrl: resourceUrl, - sourceMapUrl: sourceMapUrl(resourceUrl), - }; + try { + const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile); + return { + isInline: false, + interpolationConfig, + preserveWhitespaces, + templateUrl, + templateUrlExpression: templateUrlExpr, + resolvedTemplateUrl: resourceUrl, + sourceMapUrl: sourceMapUrl(resourceUrl), + }; + } catch (e) { + throw this.makeResourceNotFoundError( + templateUrl, templateUrlExpr, ResourceTypeForDiagnostics.Template); + } } else if (component.has('template')) { return { isInline: true, @@ -1293,33 +1278,24 @@ export class ComponentDecoratorHandler implements this.cycleAnalyzer.recordSyntheticImport(origin, imported); } - /** - * Resolve the url of a resource relative to the file that contains the reference to it. - * - * Throws a FatalDiagnosticError when unable to resolve the file. - */ - private _resolveResourceOrThrow( - file: string, basePath: string, nodeForError: ts.Node, - resourceType: ResourceTypeForDiagnostics): string { - try { - return this.resourceLoader.resolve(file, basePath); - } catch (e) { - let errorText: string; - switch (resourceType) { - case ResourceTypeForDiagnostics.Template: - errorText = `Could not find template file '${file}'.`; - break; - case ResourceTypeForDiagnostics.StylesheetFromTemplate: - errorText = `Could not find stylesheet file '${file}' linked from the template.`; - break; - case ResourceTypeForDiagnostics.StylesheetFromDecorator: - errorText = `Could not find stylesheet file '${file}'.`; - break; - } - - throw new FatalDiagnosticError( - ErrorCode.COMPONENT_RESOURCE_NOT_FOUND, nodeForError, errorText); + private makeResourceNotFoundError( + file: string, nodeForError: ts.Node, + resourceType: ResourceTypeForDiagnostics): FatalDiagnosticError { + let errorText: string; + switch (resourceType) { + case ResourceTypeForDiagnostics.Template: + errorText = `Could not find template file '${file}'.`; + break; + case ResourceTypeForDiagnostics.StylesheetFromTemplate: + errorText = `Could not find stylesheet file '${file}' linked from the template.`; + break; + case ResourceTypeForDiagnostics.StylesheetFromDecorator: + errorText = `Could not find stylesheet file '${file}'.`; + break; } + + return new FatalDiagnosticError( + ErrorCode.COMPONENT_RESOURCE_NOT_FOUND, nodeForError, errorText); } private _extractTemplateStyleUrls(template: ParsedTemplateWithSource): StyleUrlMeta[] {