Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit0aa35ec

Browse files
alxhubmhevery
authored andcommitted
fix(compiler-cli): remove the concept of an errored trait (#39967)
Previously, if a trait's analysis step resulted in diagnostics, the traitwould be considered "errored" and no further operations, including register,would be performed. Effectively, this meant that the compiler would pretendthe class in question was actually undecorated.However, this behavior is problematic for several reasons:1. It leads to inaccurate diagnostics being reported downstream.For example, if a component is put into the error state, for example due toa template error, the NgModule which declares the component would produce adiagnostic claiming that the declaration is neither a directive nor a pipe.This happened because the compiler wouldn't register() the component trait,so the component would not be recorded as actually being a directive.2. It can cause incorrect behavior on incremental builds.This bug is more complex, but the general issue is that if the compilerfails to associate a component and its module, then incremental builds willnot correctly re-analyze the module when the component's template changes.Failing to register the component as such is one link in the larger chain ofissues that result in these kinds of issues.3. It lumps together diagnostics produced during analysis and resolve steps.This is not causing issues currently as the dependency graph ensures theright classes are re-analyzed when needed, instead of showing stalediagnostics. However, the dependency graph was not intended to serve thisrole, and could potentially be optimized in ways that would break thisfunctionality.This commit removes the concept of an "errored" trait entirely from thetrait system. Instead, analyzed and resolved traits have corresponding (andseparate) diagnostics, in addition to potentially `null` analysis results.Analysis (but not resolution) diagnostics are carried forward duringincremental build operations. Compilation (emit) is only performed whena trait reaches the resolved state with no diagnostics.This change is functionally different than before as the `register` step isnow performed even in the presence of analysis errors, as long as analysisresults are also produced. This fixes problem 1 above, and is part of thelarger solution to problem 2.PRClose#39967
1 parent1e3534f commit0aa35ec

File tree

9 files changed

+164
-140
lines changed

9 files changed

+164
-140
lines changed

‎packages/compiler-cli/ngcc/src/analysis/migration_host.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,14 @@ export class DefaultMigrationHost implements MigrationHost {
3232
constmigratedTraits=this.compiler.injectSyntheticDecorator(clazz,decorator,flags);
3333

3434
for(consttraitofmigratedTraits){
35-
if(trait.state===TraitState.ERRORED){
36-
trait.diagnostics=
37-
trait.diagnostics.map(diag=>createMigrationDiagnostic(diag,clazz,decorator));
35+
if((trait.state===TraitState.Analyzed||trait.state===TraitState.Resolved)&&
36+
trait.analysisDiagnostics!==null){
37+
trait.analysisDiagnostics=trait.analysisDiagnostics.map(
38+
diag=>createMigrationDiagnostic(diag,clazz,decorator));
39+
}
40+
if(trait.state===TraitState.Resolved&&trait.resolveDiagnostics!==null){
41+
trait.resolveDiagnostics=
42+
trait.resolveDiagnostics.map(diag=>createMigrationDiagnostic(diag,clazz,decorator));
3843
}
3944
}
4045
}

‎packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ runInEachFileSystem(() => {
7979
handler.analyze.and.callFake((decl:DeclarationNode,dec:Decorator)=>{
8080
logs.push(`analyze:${(declasany).name.text}@${dec.name}`);
8181
return{
82-
analysis:{decoratorName:dec.name},
82+
analysis:!options.analyzeError ?{decoratorName:dec.name} :undefined,
8383
diagnostics:options.analyzeError ?[makeDiagnostic(9999,decl,'analyze diagnostic')] :
8484
undefined
8585
};
@@ -407,7 +407,7 @@ runInEachFileSystem(() => {
407407
`,
408408
},
409409
],
410-
{analyzeError:true,resolveError:true});
410+
{analyzeError:true,resolveError:false});
411411
analyzer.analyzeProgram();
412412
expect(diagnosticLogs.length).toEqual(1);
413413
expect(diagnosticLogs[0]).toEqual(jasmine.objectContaining({code:-999999}));

‎packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
2222
import{createComponentDecorator}from'../../src/migrations/utils';
2323
import{EntryPointBundle}from'../../src/packages/entry_point_bundle';
2424
import{makeTestEntryPointBundle}from'../helpers/utils';
25+
import{getTraitDiagnostics}from'../host/util';
2526

2627
runInEachFileSystem(()=>{
2728
describe('DefaultMigrationHost',()=>{
@@ -79,12 +80,13 @@ runInEachFileSystem(() => {
7980

8081
constrecord=compiler.recordFor(mockClazz)!;
8182
constmigratedTrait=record.traits[0];
82-
if(migratedTrait.state!==TraitState.ERRORED){
83+
constdiagnostics=getTraitDiagnostics(migratedTrait);
84+
if(diagnostics===null){
8385
returnfail('Expected migrated class trait to be in an error state');
8486
}
8587

86-
expect(migratedTrait.diagnostics.length).toBe(1);
87-
expect(ts.flattenDiagnosticMessageText(migratedTrait.diagnostics[0].messageText,'\n'))
88+
expect(diagnostics.length).toBe(1);
89+
expect(ts.flattenDiagnosticMessageText(diagnostics[0].messageText,'\n'))
8890
.toEqual(
8991
`test diagnostic\n`+
9092
` Occurs for @Component decorator inserted by an automatic migration\n`+

‎packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
1919
import{createComponentDecorator}from'../../src/migrations/utils';
2020
import{EntryPointBundle}from'../../src/packages/entry_point_bundle';
2121
import{makeTestEntryPointBundle}from'../helpers/utils';
22+
import{getTraitDiagnostics}from'../host/util';
2223

2324
runInEachFileSystem(()=>{
2425
describe('NgccTraitCompiler',()=>{
@@ -234,12 +235,13 @@ runInEachFileSystem(() => {
234235

235236
constrecord=compiler.recordFor(mockClazz)!;
236237
constmigratedTrait=record.traits[0];
237-
if(migratedTrait.state!==TraitState.ERRORED){
238+
constdiagnostics=getTraitDiagnostics(migratedTrait);
239+
if(diagnostics===null){
238240
returnfail('Expected migrated class trait to be in an error state');
239241
}
240242

241-
expect(migratedTrait.diagnostics.length).toBe(1);
242-
expect(migratedTrait.diagnostics[0].messageText).toEqual(`test diagnostic`);
243+
expect(diagnostics.length).toBe(1);
244+
expect(diagnostics[0].messageText).toEqual(`test diagnostic`);
243245
});
244246
});
245247

‎packages/compiler-cli/ngcc/test/host/util.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.io/license
77
*/
8+
import{Trait,TraitState}from'@angular/compiler-cli/src/ngtsc/transform';
89
import*astsfrom'typescript';
910
import{CtorParameter,TypeValueReferenceKind}from'../../../src/ngtsc/reflection';
1011

@@ -48,3 +49,17 @@ export function expectTypeValueReferencesForParameters(
4849
}
4950
});
5051
}
52+
53+
exportfunctiongetTraitDiagnostics(trait:Trait<unknown,unknown,unknown>):ts.Diagnostic[]|null{
54+
if(trait.state===TraitState.Analyzed){
55+
returntrait.analysisDiagnostics;
56+
}elseif(trait.state===TraitState.Resolved){
57+
constdiags=[
58+
...(trait.analysisDiagnostics??[]),
59+
...(trait.resolveDiagnostics??[]),
60+
];
61+
returndiags.length>0 ?diags :null;
62+
}else{
63+
returnnull;
64+
}
65+
}

‎packages/compiler-cli/src/ngtsc/transform/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,5 @@ export * from './src/api';
1010
export{aliasTransformFactory}from'./src/alias';
1111
export{ClassRecord,TraitCompiler}from'./src/compilation';
1212
export{declarationTransformFactory,DtsTransformRegistry,IvyDeclarationDtsTransform,ReturnTypeTransform}from'./src/declaration';
13-
export{AnalyzedTrait,ErroredTrait,PendingTrait,ResolvedTrait,SkippedTrait,Trait,TraitState}from'./src/trait';
13+
export{AnalyzedTrait,PendingTrait,ResolvedTrait,SkippedTrait,Trait,TraitState}from'./src/trait';
1414
export{ivyTransformFactory}from'./src/transform';

‎packages/compiler-cli/src/ngtsc/transform/src/compilation.ts‎

Lines changed: 47 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -181,15 +181,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
181181
consthandler=this.handlersByName.get(priorTrait.handler.name)!;
182182
lettrait:Trait<unknown,unknown,unknown>=Trait.pending(handler,priorTrait.detected);
183183

184-
if(priorTrait.state===TraitState.ANALYZED||priorTrait.state===TraitState.RESOLVED){
185-
trait=trait.toAnalyzed(priorTrait.analysis);
186-
if(trait.handler.register!==undefined){
184+
if(priorTrait.state===TraitState.Analyzed||priorTrait.state===TraitState.Resolved){
185+
trait=trait.toAnalyzed(priorTrait.analysis,priorTrait.analysisDiagnostics);
186+
if(trait.analysis!==null&&trait.handler.register!==undefined){
187187
trait.handler.register(record.node,trait.analysis);
188188
}
189-
}elseif(priorTrait.state===TraitState.SKIPPED){
189+
}elseif(priorTrait.state===TraitState.Skipped){
190190
trait=trait.toSkipped();
191-
}elseif(priorTrait.state===TraitState.ERRORED){
192-
trait=trait.toErrored(priorTrait.diagnostics);
193191
}
194192

195193
record.traits.push(trait);
@@ -314,7 +312,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
314312
preanalysis=trait.handler.preanalyze(clazz,trait.detected.metadata)||null;
315313
}catch(err){
316314
if(errinstanceofFatalDiagnosticError){
317-
trait.toErrored([err.toDiagnostic()]);
315+
trait.toAnalyzed(null,[err.toDiagnostic()]);
318316
return;
319317
}else{
320318
throwerr;
@@ -332,7 +330,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
332330
protectedanalyzeTrait(
333331
clazz:ClassDeclaration,trait:Trait<unknown,unknown,unknown>,
334332
flags?:HandlerFlags):void{
335-
if(trait.state!==TraitState.PENDING){
333+
if(trait.state!==TraitState.Pending){
336334
thrownewError(`Attempt to analyze trait of${clazz.name.text} in state${
337335
TraitState[trait.state]} (expected DETECTED)`);
338336
}
@@ -343,26 +341,18 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
343341
result=trait.handler.analyze(clazz,trait.detected.metadata,flags);
344342
}catch(err){
345343
if(errinstanceofFatalDiagnosticError){
346-
trait=trait.toErrored([err.toDiagnostic()]);
344+
trait.toAnalyzed(null,[err.toDiagnostic()]);
347345
return;
348346
}else{
349347
throwerr;
350348
}
351349
}
352350

353-
if(result.diagnostics!==undefined){
354-
trait=trait.toErrored(result.diagnostics);
355-
}elseif(result.analysis!==undefined){
356-
// Analysis was successful. Trigger registration.
357-
if(trait.handler.register!==undefined){
358-
trait.handler.register(clazz,result.analysis);
359-
}
360-
361-
// Successfully analyzed and registered.
362-
trait=trait.toAnalyzed(result.analysis);
363-
}else{
364-
trait=trait.toSkipped();
351+
if(result.analysis!==undefined&&trait.handler.register!==undefined){
352+
trait.handler.register(clazz,result.analysis);
365353
}
354+
355+
trait=trait.toAnalyzed(result.analysis??null,result.diagnostics??null);
366356
}
367357

368358
resolve():void{
@@ -372,19 +362,23 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
372362
for(lettraitofrecord.traits){
373363
consthandler=trait.handler;
374364
switch(trait.state){
375-
caseTraitState.SKIPPED:
376-
caseTraitState.ERRORED:
365+
caseTraitState.Skipped:
377366
continue;
378-
caseTraitState.PENDING:
367+
caseTraitState.Pending:
379368
thrownewError(`Resolving a trait that hasn't been analyzed:${clazz.name.text} /${
380369
Object.getPrototypeOf(trait.handler).constructor.name}`);
381-
caseTraitState.RESOLVED:
370+
caseTraitState.Resolved:
382371
thrownewError(`Resolving an already resolved trait`);
383372
}
384373

374+
if(trait.analysis===null){
375+
// No analysis results, cannot further process this trait.
376+
continue;
377+
}
378+
385379
if(handler.resolve===undefined){
386380
// No resolution of this trait needed - it's considered successful by default.
387-
trait=trait.toResolved(null);
381+
trait=trait.toResolved(null,null);
388382
continue;
389383
}
390384

@@ -393,22 +387,14 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
393387
result=handler.resolve(clazz,trait.analysisasReadonly<unknown>);
394388
}catch(err){
395389
if(errinstanceofFatalDiagnosticError){
396-
trait=trait.toErrored([err.toDiagnostic()]);
390+
trait=trait.toResolved(null,[err.toDiagnostic()]);
397391
continue;
398392
}else{
399393
throwerr;
400394
}
401395
}
402396

403-
if(result.diagnostics!==undefined&&result.diagnostics.length>0){
404-
trait=trait.toErrored(result.diagnostics);
405-
}else{
406-
if(result.data!==undefined){
407-
trait=trait.toResolved(result.data);
408-
}else{
409-
trait=trait.toResolved(null);
410-
}
411-
}
397+
trait=trait.toResolved(result.data??null,result.diagnostics??null);
412398

413399
if(result.reexports!==undefined){
414400
constfileName=clazz.getSourceFile().fileName;
@@ -436,12 +422,14 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
436422
for(constclazzofthis.fileToClasses.get(sf)!){
437423
constrecord=this.classes.get(clazz)!;
438424
for(consttraitofrecord.traits){
439-
if(trait.state!==TraitState.RESOLVED){
425+
if(trait.state!==TraitState.Resolved){
440426
continue;
441427
}elseif(trait.handler.typeCheck===undefined){
442428
continue;
443429
}
444-
trait.handler.typeCheck(ctx,clazz,trait.analysis,trait.resolution);
430+
if(trait.resolution!==null){
431+
trait.handler.typeCheck(ctx,clazz,trait.analysis,trait.resolution);
432+
}
445433
}
446434
}
447435
}
@@ -450,15 +438,17 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
450438
for(constclazzofthis.classes.keys()){
451439
constrecord=this.classes.get(clazz)!;
452440
for(consttraitofrecord.traits){
453-
if(trait.state!==TraitState.RESOLVED){
441+
if(trait.state!==TraitState.Resolved){
454442
// Skip traits that haven't been resolved successfully.
455443
continue;
456444
}elseif(trait.handler.index===undefined){
457445
// Skip traits that don't affect indexing.
458446
continue;
459447
}
460448

461-
trait.handler.index(ctx,clazz,trait.analysis,trait.resolution);
449+
if(trait.resolution!==null){
450+
trait.handler.index(ctx,clazz,trait.analysis,trait.resolution);
451+
}
462452
}
463453
}
464454
}
@@ -475,19 +465,26 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
475465
letres:CompileResult[]=[];
476466

477467
for(consttraitofrecord.traits){
478-
if(trait.state!==TraitState.RESOLVED){
468+
if(trait.state!==TraitState.Resolved||trait.analysisDiagnostics!==null||
469+
trait.resolveDiagnostics!==null){
470+
// Cannot compile a trait that is not resolved, or had any errors in its declaration.
479471
continue;
480472
}
481473

482474
constcompileSpan=this.perf.start('compileClass',original);
483475

476+
477+
// `trait.resolution` is non-null asserted here because TypeScript does not recognize that
478+
// `Readonly<unknown>` is nullable (as `unknown` itself is nullable) due to the way that
479+
// `Readonly` works.
480+
484481
letcompileRes:CompileResult|CompileResult[];
485482
if(this.compilationMode===CompilationMode.PARTIAL&&
486483
trait.handler.compilePartial!==undefined){
487-
compileRes=trait.handler.compilePartial(clazz,trait.analysis,trait.resolution);
484+
compileRes=trait.handler.compilePartial(clazz,trait.analysis,trait.resolution!);
488485
}else{
489486
compileRes=
490-
trait.handler.compileFull(clazz,trait.analysis,trait.resolution,constantPool);
487+
trait.handler.compileFull(clazz,trait.analysis,trait.resolution!,constantPool);
491488
}
492489

493490
constcompileMatchRes=compileRes;
@@ -522,7 +519,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
522519
constdecorators:ts.Decorator[]=[];
523520

524521
for(consttraitofrecord.traits){
525-
if(trait.state!==TraitState.RESOLVED){
522+
if(trait.state!==TraitState.Resolved){
526523
continue;
527524
}
528525

@@ -542,8 +539,12 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
542539
diagnostics.push(...record.metaDiagnostics);
543540
}
544541
for(consttraitofrecord.traits){
545-
if(trait.state===TraitState.ERRORED){
546-
diagnostics.push(...trait.diagnostics);
542+
if((trait.state===TraitState.Analyzed||trait.state===TraitState.Resolved)&&
543+
trait.analysisDiagnostics!==null){
544+
diagnostics.push(...trait.analysisDiagnostics);
545+
}
546+
if(trait.state===TraitState.Resolved&&trait.resolveDiagnostics!==null){
547+
diagnostics.push(...trait.resolveDiagnostics);
547548
}
548549
}
549550
}

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp