From 18e9d86a3bf8e3ea50f2c1e22d2487967f9e4dd6 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 26 Oct 2017 09:45:01 -0700 Subject: [PATCH] fix(compiler): translate emit diagnostics with `noEmitOnError: true`. (#19953) This prevents errors reported against `.ngfactory.ts` files show up as the result of running `ngc`. Closes #19935 PR Close #19953 --- .../src/diagnostics/translate_diagnostics.ts | 4 +-- .../compiler-cli/src/transformers/program.ts | 12 ++++--- .../compiler-cli/src/transformers/util.ts | 26 +++++++++++++++ packages/compiler-cli/test/ngc_spec.ts | 32 +++++++++++++++++++ .../test/transformers/program_spec.ts | 31 ++++++++++++++++++ packages/compiler/src/aot/compiler.ts | 18 +++++++---- .../src/view_compiler/type_check_compiler.ts | 13 ++++---- 7 files changed, 116 insertions(+), 20 deletions(-) diff --git a/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts b/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts index 7f5b867bdb..aff0162d8b 100644 --- a/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts +++ b/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts @@ -38,10 +38,10 @@ export function translateDiagnostics(host: TypeCheckHost, untranslatedDiagnostic source: SOURCE, code: DEFAULT_ERROR_CODE }); - return; } + } else { + ts.push(diagnostic); } - ts.push(diagnostic); }); return {ts, ng}; } diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index 3ef71c10b7..ac9a9bb1ef 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -18,7 +18,7 @@ import {CompilerHost, CompilerOptions, CustomTransformers, DEFAULT_ERROR_CODE, D import {CodeGenerator, TsCompilerAotCompilerTypeCheckHostAdapter, getOriginalReferences} from './compiler_host'; import {LowerMetadataCache, getExpressionLoweringTransformFactory} from './lower_expressions'; import {getAngularEmitterTransformFactory} from './node_emitter_transform'; -import {GENERATED_FILES, StructureIsReused, createMessageDiagnostic, isInRootDir, tsStructureIsReused} from './util'; +import {GENERATED_FILES, StructureIsReused, createMessageDiagnostic, isInRootDir, ngToTsDiagnostic, tsStructureIsReused} from './util'; @@ -149,11 +149,9 @@ class AngularCompilerProgram implements Program { getTsSemanticDiagnostics(sourceFile?: ts.SourceFile, cancellationToken?: ts.CancellationToken): ts.Diagnostic[] { - if (sourceFile) { - return this.tsProgram.getSemanticDiagnostics(sourceFile, cancellationToken); - } + const sourceFiles = sourceFile ? [sourceFile] : this.tsProgram.getSourceFiles(); let diags: ts.Diagnostic[] = []; - this.tsProgram.getSourceFiles().forEach(sf => { + sourceFiles.forEach(sf => { if (!GENERATED_FILES.test(sf.fileName)) { diags.push(...this.tsProgram.getSemanticDiagnostics(sf, cancellationToken)); } @@ -300,6 +298,10 @@ class AngularCompilerProgram implements Program { } } this.emittedSourceFiles = emittedSourceFiles; + // translate the diagnostics in the emitResult as well. + const translatedEmitDiags = translateDiagnostics(this.hostAdapter, emitResult.diagnostics); + emitResult.diagnostics = translatedEmitDiags.ts.concat( + this.structuralDiagnostics.concat(translatedEmitDiags.ng).map(ngToTsDiagnostic)); if (!outSrcMapping.length) { // if no files were emitted by TypeScript, also don't emit .json files diff --git a/packages/compiler-cli/src/transformers/util.ts b/packages/compiler-cli/src/transformers/util.ts index 07d7fb8013..7becf7547a 100644 --- a/packages/compiler-cli/src/transformers/util.ts +++ b/packages/compiler-cli/src/transformers/util.ts @@ -51,3 +51,29 @@ function pathStartsWithPrefix(prefix: string, fullPath: string): string|null { const rel = path.relative(prefix, fullPath); return rel.startsWith('..') ? null : rel; } + +/** + * Converts a ng.Diagnostic into a ts.Diagnostic. + * This looses some information, and also uses an incomplete object as `file`. + * + * I.e. only use this where the API allows only a ts.Diagnostic. + */ +export function ngToTsDiagnostic(ng: Diagnostic): ts.Diagnostic { + let file: ts.SourceFile|undefined; + let start: number|undefined; + let length: number|undefined; + if (ng.span) { + // Note: We can't use a real ts.SourceFile, + // but we can at least mirror the properties `fileName` and `text`, which + // are mostly used for error reporting. + file = { fileName: ng.span.start.file.url, text: ng.span.start.file.content } as ts.SourceFile; + start = ng.span.start.offset; + length = ng.span.end.offset - start; + } + return { + file, + messageText: ng.messageText, + category: ng.category, + code: ng.code, start, length, + }; +} diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index d51b49fdc4..bf90700146 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -1467,5 +1467,37 @@ describe('ngc transformer command-line', () => { expect(exitCode).toBe(2, 'Compile was expected to fail'); expect(messages[0]).toContain(['Tagged template expressions are not supported in metadata']); }); + + it('should allow using 2 classes with the same name in declarations with noEmitOnError=true', + () => { + write('src/tsconfig.json', `{ + "extends": "../tsconfig-base.json", + "compilerOptions": { + "noEmitOnError": true + }, + "files": ["test-module.ts"] + }`); + function writeComp(fileName: string) { + write(fileName, ` + import {Component} from '@angular/core'; + + @Component({selector: 'comp', template: ''}) + export class TestComponent {} + `); + } + writeComp('src/comp1.ts'); + writeComp('src/comp2.ts'); + write('src/test-module.ts', ` + import {NgModule} from '@angular/core'; + import {TestComponent as Comp1} from './comp1'; + import {TestComponent as Comp2} from './comp2'; + + @NgModule({ + declarations: [Comp1, Comp2], + }) + export class MyModule {} + `); + expect(main(['-p', path.join(basePath, 'src/tsconfig.json')])).toBe(0); + }); }); }); diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index 19c945d101..8bc67cfbf5 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as ts from 'typescript'; +import {formatDiagnostics} from '../../src/perform_compile'; import {CompilerHost, EmitFlags, LazyRoute} from '../../src/transformers/api'; import {createSrcToOutPathMapper} from '../../src/transformers/program'; import {GENERATED_FILES, StructureIsReused, tsStructureIsReused} from '../../src/transformers/util'; @@ -858,4 +859,34 @@ describe('ng program', () => { }]); }); }); + + it('should report errors for ts and ng errors on emit with noEmitOnError=true', () => { + testSupport.writeFiles({ + 'src/main.ts': ` + import {Component, NgModule} from '@angular/core'; + + // Ts error + let x: string = 1; + + // Ng error + @Component({selector: 'comp', templateUrl: './main.html'}) + export class MyComp {} + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `, + 'src/main.html': '{{nonExistent}}' + }); + const options = testSupport.createCompilerOptions({noEmitOnError: true}); + const host = ng.createCompilerHost({options}); + const program1 = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/main.ts')], options, host}); + const errorDiags = + program1.emit().diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error); + expect(formatDiagnostics(errorDiags)) + .toContain(`src/main.ts(5,13): error TS2322: Type '1' is not assignable to type 'string'.`); + expect(formatDiagnostics(errorDiags)) + .toContain( + `src/main.html(1,1): error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`); + }); }); diff --git a/packages/compiler/src/aot/compiler.ts b/packages/compiler/src/aot/compiler.ts index 3e2eda7ef5..22be0661ae 100644 --- a/packages/compiler/src/aot/compiler.ts +++ b/packages/compiler/src/aot/compiler.ts @@ -193,6 +193,7 @@ export class AotCompiler { private _createNgFactoryStub( outputCtx: OutputContext, file: NgAnalyzedFile, emitFlags: StubEmitFlags) { + let componentId = 0; file.ngModules.forEach((ngModuleMeta, ngModuleIndex) => { // Note: the code below needs to executed for StubEmitFlags.Basic and StubEmitFlags.TypeCheck, // so we don't change the .ngfactory file too much when adding the typecheck block. @@ -230,12 +231,14 @@ export class AotCompiler { if (!compMeta.isComponent) { return; } + componentId++; this._createTypeCheckBlock( - outputCtx, ngModuleMeta, this._metadataResolver.getHostComponentMetadata(compMeta), - [compMeta.type], externalReferenceVars); - this._createTypeCheckBlock( - outputCtx, ngModuleMeta, compMeta, ngModuleMeta.transitiveModule.directives, + outputCtx, `${compMeta.type.reference.name}_Host_${componentId}`, ngModuleMeta, + this._metadataResolver.getHostComponentMetadata(compMeta), [compMeta.type], externalReferenceVars); + this._createTypeCheckBlock( + outputCtx, `${compMeta.type.reference.name}_${componentId}`, ngModuleMeta, compMeta, + ngModuleMeta.transitiveModule.directives, externalReferenceVars); }); } }); @@ -246,12 +249,13 @@ export class AotCompiler { } private _createTypeCheckBlock( - ctx: OutputContext, moduleMeta: CompileNgModuleMetadata, compMeta: CompileDirectiveMetadata, - directives: CompileIdentifierMetadata[], externalReferenceVars: Map) { + ctx: OutputContext, componentId: string, moduleMeta: CompileNgModuleMetadata, + compMeta: CompileDirectiveMetadata, directives: CompileIdentifierMetadata[], + externalReferenceVars: Map) { const {template: parsedTemplate, pipes: usedPipes} = this._parseTemplate(compMeta, moduleMeta, directives); ctx.statements.push(...this._typeCheckCompiler.compileComponent( - compMeta, parsedTemplate, usedPipes, externalReferenceVars)); + componentId, compMeta, parsedTemplate, usedPipes, externalReferenceVars)); } emitMessageBundle(analyzeResult: NgAnalyzedModules, locale: string|null): MessageBundle { diff --git a/packages/compiler/src/view_compiler/type_check_compiler.ts b/packages/compiler/src/view_compiler/type_check_compiler.ts index 9e5f58c6f3..3b4597979e 100644 --- a/packages/compiler/src/view_compiler/type_check_compiler.ts +++ b/packages/compiler/src/view_compiler/type_check_compiler.ts @@ -9,7 +9,7 @@ import {AotCompilerOptions} from '../aot/compiler_options'; import {StaticReflector} from '../aot/static_reflector'; import {StaticSymbol} from '../aot/static_symbol'; -import {CompileDiDependencyMetadata, CompileDirectiveMetadata, CompilePipeSummary, viewClassName} from '../compile_metadata'; +import {CompileDiDependencyMetadata, CompileDirectiveMetadata, CompilePipeSummary} from '../compile_metadata'; import {BuiltinConverter, EventHandlerVars, LocalResolver, convertActionBinding, convertPropertyBinding, convertPropertyBindingBuiltins} from '../compiler_util/expression_converter'; import {AST, ASTWithSource, Interpolation} from '../expression_parser/ast'; import {Identifiers} from '../identifiers'; @@ -33,7 +33,8 @@ export class TypeCheckCompiler { * and also violate the point above. */ compileComponent( - component: CompileDirectiveMetadata, template: TemplateAst[], usedPipes: CompilePipeSummary[], + componentId: string, component: CompileDirectiveMetadata, template: TemplateAst[], + usedPipes: CompilePipeSummary[], externalReferenceVars: Map): o.Statement[] { const pipes = new Map(); usedPipes.forEach(p => pipes.set(p.name, p.type.reference)); @@ -48,7 +49,7 @@ export class TypeCheckCompiler { const visitor = viewBuilderFactory(null); visitor.visitAll([], template); - return visitor.build(); + return visitor.build(componentId); } } @@ -103,8 +104,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { templateVisitAll(this, astNodes); } - build(targetStatements: o.Statement[] = []): o.Statement[] { - this.children.forEach((child) => child.build(targetStatements)); + build(componentId: string, targetStatements: o.Statement[] = []): o.Statement[] { + this.children.forEach((child) => child.build(componentId, targetStatements)); const viewStmts: o.Statement[] = [o.variable(DYNAMIC_VAR_NAME).set(o.NULL_EXPR).toDeclStmt(o.DYNAMIC_TYPE)]; let bindingCount = 0; @@ -128,7 +129,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { (stmt: o.Statement) => o.applySourceSpanToStatementIfNeeded(stmt, sourceSpan))); }); - const viewName = `_View_${this.component.name}_${this.embeddedViewIndex}`; + const viewName = `_View_${componentId}_${this.embeddedViewIndex}`; const viewFactory = new o.DeclareFunctionStmt(viewName, [], viewStmts); targetStatements.push(viewFactory); return targetStatements;