Skip to content

Commit

Permalink
fix(compiler-cli): correctly detect deferred dependencies across scop…
Browse files Browse the repository at this point in the history
…ed nodes (#54499)

This is based on an internal issue report.

An earlier change introduced a diagnostic to report cases where a symbol is in the `deferredImports` array, but is used eagerly. The check worked by looking through the deferred blocks in a scope, resolving the scope for each and checking if the element is within the scope. The problem is that resolving the scope won't work across scoped node boundaries. For example, if there's a control flow statement around the block or within the block but around the deferred dependency, it won't be able to resolve the scope since it isn't a direct child, e.g.

```
@if (true) {
  @defer {
   <deferred-dep/>
  }
}
```

To fix this the case where the deferred block is inside a scoped node, I've changed the `R3BoundTarget.deferBlocks` to be a `Map` holding both the deferred block and its corresponding scope. Then to resolve the case where the dependency is within a scoped node inside the deferred block, I've added a depth-first traversal through the scopes within the deferred block.

PR Close #54499
  • Loading branch information
crisbeto authored and alxhub committed Feb 21, 2024
1 parent 1d14e52 commit badda0c
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 13 deletions.
80 changes: 80 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -10063,6 +10063,86 @@ function allTests(os: string) {
expect(diags.length).toBe(1);
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.DEFERRED_PIPE_USED_EAGERLY));
});

it('should not produce an error when a deferred block is wrapped in a conditional',
() => {
env.write('deferred-a.ts', `
import {Component} from '@angular/core';
@Component({
standalone: true,
selector: 'deferred-cmp-a',
template: 'DeferredCmpA contents',
})
export class DeferredCmpA {
}
`);

env.write('test.ts', `
import {Component} from '@angular/core';
import {DeferredCmpA} from './deferred-a';
@Component({
standalone: true,
deferredImports: [DeferredCmpA],
template: \`
@if (true) {
@if (true) {
@if (true) {
@defer {
<deferred-cmp-a />
}
}
}
}
\`,
})
export class AppCmp {
condition = true;
}
`);

const diags = env.driveDiagnostics();
expect(diags).toEqual([]);
});

it('should not produce an error when a dependency is wrapped in a condition inside of a deferred block',
() => {
env.write('deferred-a.ts', `
import {Component} from '@angular/core';
@Component({
standalone: true,
selector: 'deferred-cmp-a',
template: 'DeferredCmpA contents',
})
export class DeferredCmpA {
}
`);

env.write('test.ts', `
import {Component} from '@angular/core';
import {DeferredCmpA} from './deferred-a';
@Component({
standalone: true,
deferredImports: [DeferredCmpA],
template: \`
@defer {
@if (true) {
@if (true) {
@if (true) {
<deferred-cmp-a />
}
}
}
}
\`,
})
export class AppCmp {
condition = true;
}
`);

const diags = env.driveDiagnostics();
expect(diags).toEqual([]);
});
});
});

Expand Down
40 changes: 27 additions & 13 deletions packages/compiler/src/render3/view/t2_binder.ts
Expand Up @@ -51,7 +51,7 @@ export class R3TargetBinder<DirectiveT extends DirectiveMeta> implements TargetB
TemplateBinder.applyWithScope(target.template, scope);
return new R3BoundTarget(
target, directives, eagerDirectives, bindings, references, expressions, symbols,
nestingLevel, scopedNodeEntities, usedPipes, eagerPipes, deferBlocks, scope);
nestingLevel, scopedNodeEntities, usedPipes, eagerPipes, deferBlocks);
}
}

Expand Down Expand Up @@ -472,7 +472,7 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
private constructor(
private bindings: Map<AST, Reference|Variable>,
private symbols: Map<Reference|Variable, ScopedNode>, private usedPipes: Set<string>,
private eagerPipes: Set<string>, private deferBlocks: Set<DeferredBlock>,
private eagerPipes: Set<string>, private deferBlocks: Map<DeferredBlock, Scope>,
private nestingLevel: Map<ScopedNode, number>, private scope: Scope,
private rootNode: ScopedNode|null, private level: number) {
super();
Expand Down Expand Up @@ -510,15 +510,15 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
nestingLevel: Map<ScopedNode, number>,
usedPipes: Set<string>,
eagerPipes: Set<string>,
deferBlocks: Set<DeferredBlock>,
deferBlocks: Map<DeferredBlock, Scope>,
} {
const expressions = new Map<AST, Reference|Variable>();
const symbols = new Map<Variable|Reference, Template>();
const nestingLevel = new Map<ScopedNode, number>();
const usedPipes = new Set<string>();
const eagerPipes = new Set<string>();
const template = nodes instanceof Template ? nodes : null;
const deferBlocks = new Set<DeferredBlock>();
const deferBlocks = new Map<DeferredBlock, Scope>();
// The top-level template has nesting level 0.
const binder = new TemplateBinder(
expressions, symbols, usedPipes, eagerPipes, deferBlocks, nestingLevel, scope, template, 0);
Expand Down Expand Up @@ -547,9 +547,17 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
nodeOrNodes.trackBy.visit(this);
nodeOrNodes.children.forEach(this.visitNode);
this.nestingLevel.set(nodeOrNodes, this.level);
} else if (nodeOrNodes instanceof DeferredBlock) {
if (this.scope.rootNode !== nodeOrNodes) {
throw new Error(
`Assertion error: resolved incorrect scope for deferred block ${nodeOrNodes}`);
}
this.deferBlocks.set(nodeOrNodes, this.scope);
nodeOrNodes.children.forEach(node => node.visit(this));
this.nestingLevel.set(nodeOrNodes, this.level);
} else if (
nodeOrNodes instanceof SwitchBlockCase || nodeOrNodes instanceof ForLoopBlockEmpty ||
nodeOrNodes instanceof DeferredBlock || nodeOrNodes instanceof DeferredBlockError ||
nodeOrNodes instanceof DeferredBlockError ||
nodeOrNodes instanceof DeferredBlockPlaceholder ||
nodeOrNodes instanceof DeferredBlockLoading) {
nodeOrNodes.children.forEach(node => node.visit(this));
Expand Down Expand Up @@ -616,9 +624,7 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
}

visitDeferredBlock(deferred: DeferredBlock) {
this.deferBlocks.add(deferred);
this.ingestScopedNode(deferred);

deferred.triggers.when?.value.visit(this);
deferred.prefetchTriggers.when?.value.visit(this);
deferred.placeholder && this.visitNode(deferred.placeholder);
Expand Down Expand Up @@ -738,7 +744,7 @@ export class R3BoundTarget<DirectiveT extends DirectiveMeta> implements BoundTar
private nestingLevel: Map<ScopedNode, number>,
private scopedNodeEntities: Map<ScopedNode|null, ReadonlySet<Reference|Variable>>,
private usedPipes: Set<string>, private eagerPipes: Set<string>,
private deferredBlocks: Set<DeferredBlock>, private rootScope: Scope) {}
private deferBlocks: Map<DeferredBlock, Scope>) {}

getEntitiesInScope(node: ScopedNode|null): ReadonlySet<Reference|Variable> {
return this.scopedNodeEntities.get(node) ?? new Set();
Expand Down Expand Up @@ -789,7 +795,7 @@ export class R3BoundTarget<DirectiveT extends DirectiveMeta> implements BoundTar
}

getDeferBlocks(): DeferredBlock[] {
return Array.from(this.deferredBlocks);
return Array.from(this.deferBlocks.keys());
}

getDeferredTriggerTarget(block: DeferredBlock, trigger: DeferredTrigger): Element|null {
Expand Down Expand Up @@ -856,12 +862,20 @@ export class R3BoundTarget<DirectiveT extends DirectiveMeta> implements BoundTar
}

isDeferred(element: Element): boolean {
for (const deferBlock of this.deferredBlocks) {
const scope = this.rootScope.childScopes.get(deferBlock);
if (scope && scope.elementsInScope.has(element)) {
return true;
for (const deferredScope of this.deferBlocks.values()) {
const stack = [deferredScope];

while (stack.length > 0) {
const current = stack.pop()!;

if (current.elementsInScope.has(element)) {
return true;
}

stack.push(...current.childScopes.values());
}
}

return false;
}

Expand Down

0 comments on commit badda0c

Please sign in to comment.