Skip to content
Permalink
Browse files

fix(ngcc): report diagnostics from migrations (#34014)

When ngcc is analyzing synthetically inserted decorators from a
migration, it is typically not expected that any diagnostics are
produced. In the situation where a diagnostic is produced, however, the
diagnostic would not be reported at all. This commit ensures that
diagnostics in migrations are reported.

PR Close #34014
  • Loading branch information
JoostK authored and AndrewKushnir committed Nov 23, 2019
1 parent 60051eb commit 599dcd0d85f930555af54c5b7b495fdc9bd099d2
@@ -160,7 +160,7 @@ export class DecorationAnalyzer {
protected applyMigrations(analyzedFiles: AnalyzedFile[]): void {
const migrationHost = new DefaultMigrationHost(
this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers,
this.bundle.entryPoint.path, analyzedFiles);
this.bundle.entryPoint.path, analyzedFiles, this.diagnosticHandler);

this.migrations.forEach(migration => {
analyzedFiles.forEach(analyzedFile => {
@@ -27,7 +27,8 @@ export class DefaultMigrationHost implements MigrationHost {
constructor(
readonly reflectionHost: NgccReflectionHost, readonly metadata: MetadataReader,
readonly evaluator: PartialEvaluator, private handlers: DecoratorHandler<any, any>[],
private entryPointPath: AbsoluteFsPath, private analyzedFiles: AnalyzedFile[]) {}
private entryPointPath: AbsoluteFsPath, private analyzedFiles: AnalyzedFile[],
private diagnosticHandler: (error: ts.Diagnostic) => void) {}

injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags):
void {
@@ -37,6 +38,12 @@ export class DefaultMigrationHost implements MigrationHost {
return;
}

if (newAnalyzedClass.diagnostics !== undefined) {
for (const diagnostic of newAnalyzedClass.diagnostics) {
this.diagnosticHandler(createMigrationDiagnostic(diagnostic, clazz, decorator));
}
}

const analyzedFile = getOrCreateAnalyzedFile(this.analyzedFiles, clazz.getSourceFile());
const oldAnalyzedClass = analyzedFile.analyzedClasses.find(c => c.declaration === clazz);
if (oldAnalyzedClass === undefined) {
@@ -102,3 +109,43 @@ function mergeAnalyzedClasses(oldClass: AnalyzedClass, newClass: AnalyzedClass)
}
}
}

/**
* Creates a diagnostic from another one, containing additional information about the synthetic
* decorator.
*/
function createMigrationDiagnostic(
diagnostic: ts.Diagnostic, source: ts.Node, decorator: Decorator): ts.Diagnostic {
const clone = {...diagnostic};

const chain: ts.DiagnosticMessageChain[] = [{
messageText: `Occurs for @${decorator.name} decorator inserted by an automatic migration`,
category: ts.DiagnosticCategory.Message,
code: 0,
}];

if (decorator.args !== null) {
const args = decorator.args.map(arg => arg.getText()).join(', ');
chain.push({
messageText: `@${decorator.name}(${args})`,
category: ts.DiagnosticCategory.Message,
code: 0,
});
}

if (typeof clone.messageText === 'string') {
clone.messageText = {
messageText: clone.messageText,
category: diagnostic.category,
code: diagnostic.code,
next: chain,
};
} else {
if (clone.messageText.next === undefined) {
clone.messageText.next = chain;
} else {
clone.messageText.next.push(...chain);
}
}
return clone;
}
@@ -5,14 +5,17 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {ErrorCode} from '../../../src/ngtsc/diagnostics';
import * as ts from 'typescript';

import {ErrorCode, makeDiagnostic} from '../../../src/ngtsc/diagnostics';
import {AbsoluteFsPath, absoluteFrom} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection';
import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../src/ngtsc/transform';
import {DefaultMigrationHost} from '../../src/analysis/migration_host';
import {AnalyzedClass, AnalyzedFile} from '../../src/analysis/types';
import {NgccClassSymbol} from '../../src/host/ngcc_host';
import {createComponentDecorator} from '../../src/migrations/utils';

runInEachFileSystem(() => {
describe('DefaultMigrationHost', () => {
@@ -23,6 +26,7 @@ runInEachFileSystem(() => {
let mockEvaluator: any = {};
let mockClazz: any;
let mockDecorator: any = {name: 'MockDecorator'};
let diagnosticHandler = () => {};
beforeEach(() => {
_ = absoluteFrom;
entryPointPath = _('/node_modules/some-package/entry-point');
@@ -51,7 +55,8 @@ runInEachFileSystem(() => {
const handler1 = new TestHandler('handler1', log);
const handler2 = new TestHandler('handler2', log);
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler1, handler2], entryPointPath, []);
mockHost, mockMetadata, mockEvaluator, [handler1, handler2], entryPointPath, [],
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(log).toEqual([
`handler1:detect:MockClazz:MockDecorator`,
@@ -67,7 +72,7 @@ runInEachFileSystem(() => {
const handler3 = new TestHandler('handler3', log);
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler1, handler2, handler3],
entryPointPath, []);
entryPointPath, [], diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(log).toEqual([
`handler1:detect:MockClazz:MockDecorator`,
@@ -82,7 +87,8 @@ runInEachFileSystem(() => {
const handler = new AlwaysDetectHandler('handler', log);
const analyzedFiles: AnalyzedFile[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(analyzedFiles.length).toEqual(1);
expect(analyzedFiles[0].analyzedClasses.length).toEqual(1);
@@ -99,7 +105,8 @@ runInEachFileSystem(() => {
analyzedClasses: [DUMMY_CLASS_1, DUMMY_CLASS_2],
}];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(analyzedFiles.length).toEqual(1);
expect(analyzedFiles[0].analyzedClasses.length).toEqual(3);
@@ -120,7 +127,8 @@ runInEachFileSystem(() => {
analyzedClasses: [analyzedClass],
}];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(analyzedFiles.length).toEqual(1);
expect(analyzedFiles[0].analyzedClasses.length).toEqual(1);
@@ -144,7 +152,8 @@ runInEachFileSystem(() => {
analyzedClasses: [analyzedClass],
}];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);
expect(analyzedFiles.length).toEqual(1);
expect(analyzedFiles[0].analyzedClasses.length).toEqual(1);
@@ -167,11 +176,34 @@ runInEachFileSystem(() => {
analyzedClasses: [analyzedClass],
}];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
expect(() => host.injectSyntheticDecorator(mockClazz, mockDecorator))
.toThrow(jasmine.objectContaining(
{code: ErrorCode.NGCC_MIGRATION_DECORATOR_INJECTION_ERROR}));
});

it('should report diagnostics from handlers', () => {
const log: string[] = [];
const handler = new DiagnosticProducingHandler('handler', log);
const analyzedFiles: AnalyzedFile[] = [];
const diagnostics: ts.Diagnostic[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnostic => diagnostics.push(diagnostic));
mockClazz.getStart = () => 0;
mockClazz.getWidth = () => 0;

const decorator = createComponentDecorator(mockClazz, {selector: 'comp', exportAs: null});
host.injectSyntheticDecorator(mockClazz, decorator);

expect(diagnostics.length).toBe(1);
expect(ts.flattenDiagnosticMessageText(diagnostics[0].messageText, '\n'))
.toEqual(
`test diagnostic\n` +
` Occurs for @Component decorator inserted by an automatic migration\n` +
` @Component({ template: "", selector: "comp" })`);
});
});

describe('getAllDecorators', () => {
@@ -180,7 +212,8 @@ runInEachFileSystem(() => {
const handler = new AlwaysDetectHandler('handler', log);
const analyzedFiles: AnalyzedFile[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);

const decorators = host.getAllDecorators(mockClazz);
expect(decorators).toBeNull();
@@ -191,7 +224,8 @@ runInEachFileSystem(() => {
const handler = new AlwaysDetectHandler('handler', log);
const analyzedFiles: AnalyzedFile[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);

const sourceFile: any = {};
const unrelatedClass: any = {
@@ -218,7 +252,8 @@ runInEachFileSystem(() => {
analyzedClasses: [analyzedClass],
}];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles,
diagnosticHandler);
host.injectSyntheticDecorator(mockClazz, mockDecorator);

const decorators = host.getAllDecorators(mockClazz) !;
@@ -232,7 +267,8 @@ runInEachFileSystem(() => {
it('should be true for nodes within the entry-point', () => {
const analyzedFiles: AnalyzedFile[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles,
diagnosticHandler);

const sourceFile: any = {
fileName: _('/node_modules/some-package/entry-point/relative.js'),
@@ -246,7 +282,8 @@ runInEachFileSystem(() => {
it('should be false for nodes outside the entry-point', () => {
const analyzedFiles: AnalyzedFile[] = [];
const host = new DefaultMigrationHost(
mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles);
mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles,
diagnosticHandler);

const sourceFile: any = {
fileName: _('/node_modules/some-package/other-entry/index.js'),
@@ -284,3 +321,10 @@ class AlwaysDetectHandler extends TestHandler {
return {trigger: node, metadata: {}};
}
}

class DiagnosticProducingHandler extends AlwaysDetectHandler {
analyze(node: ClassDeclaration): AnalysisOutput<any> {
super.analyze(node);
return {diagnostics: [makeDiagnostic(9999, node, 'test diagnostic')]};
}
}

0 comments on commit 599dcd0

Please sign in to comment.
You can’t perform that action at this time.