fix(compiler-cli): use correct module import for types behind a forwardRef
#42887
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
…ardRef` The static interpreter assumed that a foreign function expression would have to be imported from the absolute module specifier that was used for the foreign function itself. This assumption does not hold for the `forwardRef` foreign function resolver, as that extracts the resolved expression from the function's argument, which is not behind the absolute module import of the `forwardRef` function. The prior behavior has worked for the typical usage of `forwardRef`, when it is contained within the same source file as where the static evaluation started. In that case, the resulting reference would incorrectly have an absolute module guess of `@angular/core`, but the local identifier emit strategy was capable of emitting the reference without generating an import using the absolute module guess. In the scenario where the static interpreter would first have to follow a reference to a different source that contained the `forwardRef` would the compilation fail. In that case, there is no local identifier available such that the absolute module emitter would try to locate the imported symbol from `@angular/core`. which fails as the symbol is not exported from there. This commit fixes the issue by checking whether a foreign expression occurs in the same source file as the call expression. If it does, then the absolute module specifier that was used to resolve the call expression is ignored. Fixes angular#42865
JoostK
added
target: patch
JoostK
added
the
action: review
packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts
Outdated
Show resolved
Hide resolved
petebacondarwin
approved these changes
Jul 19, 2021
Comment on lines
658
to
661
| (_ref, args) => { | ||
| // Extracts the `Foo` from `() => Foo`. | ||
| return (args[0] as ts.ArrowFunction).body as ts.Expression; | ||
| }); |
There was a problem hiding this comment.
Similar to firstArgFfr it would make this test easier to follow if you moved this inline function to a named function.
Comment on lines
680
to
684
| const resolved = evaluator.evaluate(expression, (ref) => { | ||
| // Extract the `Foo` from the return type of the `external` function declaration. | ||
| return ((ref.node as ts.FunctionDeclaration).type as ts.TypeReferenceNode).typeName as | ||
| ts.Identifier; | ||
| }); |
There was a problem hiding this comment.
Similarly, it would be easier to follow if this was a named function.
petebacondarwin
added
action: cleanup
JoostK
added
action: merge
AndrewKushnir
removed
the
action: presubmit
alxhub
pushed a commit
that referenced
this pull request
Jul 20, 2021
…ardRef` (#42887) The static interpreter assumed that a foreign function expression would have to be imported from the absolute module specifier that was used for the foreign function itself. This assumption does not hold for the `forwardRef` foreign function resolver, as that extracts the resolved expression from the function's argument, which is not behind the absolute module import of the `forwardRef` function. The prior behavior has worked for the typical usage of `forwardRef`, when it is contained within the same source file as where the static evaluation started. In that case, the resulting reference would incorrectly have an absolute module guess of `@angular/core`, but the local identifier emit strategy was capable of emitting the reference without generating an import using the absolute module guess. In the scenario where the static interpreter would first have to follow a reference to a different source that contained the `forwardRef` would the compilation fail. In that case, there is no local identifier available such that the absolute module emitter would try to locate the imported symbol from `@angular/core`. which fails as the symbol is not exported from there. This commit fixes the issue by checking whether a foreign expression occurs in the same source file as the call expression. If it does, then the absolute module specifier that was used to resolve the call expression is ignored. Fixes #42865 PR Close #42887
|
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.
The static interpreter assumed that a foreign function expression would
have to be imported from the absolute module specifier that was used for
the foreign function itself. This assumption does not hold for the
forwardRefforeign function resolver, as that extracts the resolvedexpression from the function's argument, which is not behind the
absolute module import of the
forwardReffunction.The prior behavior has worked for the typical usage of
forwardRef,when it is contained within the same source file as where the static
evaluation started. In that case, the resulting reference would
incorrectly have an absolute module guess of
@angular/core, but thelocal identifier emit strategy was capable of emitting the reference
without generating an import using the absolute module guess.
In the scenario where the static interpreter would first have to follow
a reference to a different source that contained the
forwardRefwouldthe compilation fail. In that case, there is no local identifier
available such that the absolute module emitter would try to locate the
imported symbol from
@angular/core. which fails as the symbol is notexported from there.
This commit fixes the issue by checking whether a foreign expression
occurs in the same source file as the call expression. If it does, then
the absolute module specifier that was used to resolve the call
expression is ignored.
Fixes #42865