Correctly detect deferred dependencies inside scoped node and reorganize defer tests #54499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crisbeto
added
action: review
AndrewKushnir
approved these changes
Feb 19, 2024
AndrewKushnir
removed
the
action: review
alxhub
added
action: cleanup
…ed nodes 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.
Splits the tests for `@defer` blocks out into a separate file since the `ngtsc_spec.ts` is getting quite large.
crisbeto
force-pushed
the
deferred-scope-check
branch
from
649efd5
to
7bccc28
Compare
crisbeto
added
action: merge
|
This PR was merged into the repository by commit a9f563f. |
alxhub
pushed a commit
that referenced
this pull request
Feb 21, 2024
…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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note for reviewer: it may be easier to look through this commit-by-commit since I took the chance to reorganize our tests a bit so they're easier to navigate. Includes the following changes:
fix(compiler-cli): correctly detect deferred dependencies across scoped nodes
This is based on an internal issue report.
An earlier change introduced a diagnostic to report cases where a symbol is in the
deferredImportsarray, 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.To fix this the case where the deferred block is inside a scoped node, I've changed the
R3BoundTarget.deferBlocksto be aMapholding 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.refactor(compiler-cli): move defer block tests into separate file
Splits the tests for
@deferblocks out into a separate file since thengtsc_spec.tsis getting quite large.