Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler-cli): various improvements to import generation logic #44587

Closed
wants to merge 7 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Dec 30, 2021

See individual commits.

Closes #44414
Fixes #43624

@ngbot ngbot bot added this to the Backlog milestone Dec 30, 2021
@ngbot ngbot bot added this to the Backlog milestone Dec 30, 2021
@JoostK JoostK changed the title Ngtsc/fallible reference emit fix(compiler-cli): various improvements to import generation logic Dec 30, 2021
@JoostK JoostK force-pushed the ngtsc/fallible-reference-emit branch from 6dd5103 to dd9e5d7 Compare Dec 30, 2021
@JoostK JoostK force-pushed the ngtsc/fallible-reference-emit branch from dd9e5d7 to 18c298e Compare Dec 30, 2021
@JoostK JoostK marked this pull request as ready for review Dec 30, 2021
alxhub
alxhub approved these changes Jan 4, 2022
Copy link
Contributor

@alxhub alxhub left a comment

👍 some excellent compiler improvements, thanks @JoostK!

@pullapprove pullapprove bot requested a review from alxhub Jan 4, 2022
Copy link
Member

@jelbourn jelbourn left a comment

LGTM

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from atscott Jan 5, 2022
JoostK added 7 commits Jan 5, 2022
In certain scenarios, the compiler may have crashed with an
`Unable to write a reference` error which would be particularly hard
to diagnose. One of the primary reasons for this failure is when the
`rootDir` option is configured---typically the case for libraries---
and a source file is imported using a relative import from an external
entry-point. This would normally report TS6059 for the invalid relative
import, but the crash prevents this error from being surfaced.

This commit refactors the reference emit logic to result in an explicit
`Failure` state with a reason as to why the failure occurred. This state
is then used to report a `FatalDiagnosticException`, preventing a hard
crash.

Closes angular#44414
…ce file path

Using `absoluteFromSourceFile` leverages the cache of the resolved
absolute path, instead of having to compute it each time.
This commit fixes an issue with symlink handling in `MockFileSystem`,
where entries within a symlink would fail to resolve.
The `NgtscCompilerHost` is implemented using the `FileSystem`
abstraction of the compiler, which is implemented for tests using an
in-memory `MockFileSystem`. If the in-memory filesystem contains
symlinks, then using `NgtscCompilerHost` would not reflect their
resolved real path. Instead, the TypeScript compiler would use its
default implementation based on the real filesystem, which is unaware of
the in-memory `MockFileSystem` setup.

This change does not currently address any issues, but is being fixed
as it prevented a reproduction scenario from behaving correctly.
When building a library, the `rootDir` option is configured to ensure
that all source files are present within the entry-point that is being
build. This imposes an extra constraint on the reference emit logic,
which does not allow emitting a reference into a source file outside of
this `rootDir`.

During the generation of type-check blocks we used to make a best-effort
estimation of whether a type reference can be emitted into the
type-check file. This check was relaxed in angular#42492 to support emitting
more syntax forms and type references, but this change did not consider
the `rootDir` constraint that is present in library builds. As such, the
compiler might conclude that a type reference is eligible for emit into
the type-check file, whereas in practice this would cause a failure.

This commit changes the best-effort estimation into a "preflight"
reference emit that is fully accurate as to whether emitting a type
reference is possible.

Fixes angular#43624
@JoostK JoostK force-pushed the ngtsc/fallible-reference-emit branch from 18c298e to 118ddcc Compare Jan 5, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Reviewed-for: public-api

atscott
atscott approved these changes Jan 5, 2022
Copy link
Contributor

@atscott atscott left a comment

reviewed-for: public-api

@atscott
Copy link
Contributor

@atscott atscott commented Jan 5, 2022

alxhub
alxhub approved these changes Jan 6, 2022
Copy link
Contributor

@alxhub alxhub left a comment

Reviewed-for: public-api

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

LGTM 🍪

reviewed-for: public-api

@JoostK
Copy link
Member Author

@JoostK JoostK commented Jan 6, 2022

caretaker note: please review the presubmit failure and merge if possible, as I hope they're flakes.

@atscott
Copy link
Contributor

@atscott atscott commented Jan 6, 2022

This PR was merged into the repository by commit f8af49e.

atscott added a commit that referenced this issue Jan 6, 2022
…ce file path (#44587)

Using `absoluteFromSourceFile` leverages the cache of the resolved
absolute path, instead of having to compute it each time.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…4587)

This commit fixes an issue with symlink handling in `MockFileSystem`,
where entries within a symlink would fail to resolve.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…44587)

The `NgtscCompilerHost` is implemented using the `FileSystem`
abstraction of the compiler, which is implemented for tests using an
in-memory `MockFileSystem`. If the in-memory filesystem contains
symlinks, then using `NgtscCompilerHost` would not reflect their
resolved real path. Instead, the TypeScript compiler would use its
default implementation based on the real filesystem, which is unaware of
the in-memory `MockFileSystem` setup.

This change does not currently address any issues, but is being fixed
as it prevented a reproduction scenario from behaving correctly.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…44587)

When building a library, the `rootDir` option is configured to ensure
that all source files are present within the entry-point that is being
build. This imposes an extra constraint on the reference emit logic,
which does not allow emitting a reference into a source file outside of
this `rootDir`.

During the generation of type-check blocks we used to make a best-effort
estimation of whether a type reference can be emitted into the
type-check file. This check was relaxed in #42492 to support emitting
more syntax forms and type references, but this change did not consider
the `rootDir` constraint that is present in library builds. As such, the
compiler might conclude that a type reference is eligible for emit into
the type-check file, whereas in practice this would cause a failure.

This commit changes the best-effort estimation into a "preflight"
reference emit that is fully accurate as to whether emitting a type
reference is possible.

Fixes #43624

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
In certain scenarios, the compiler may have crashed with an
`Unable to write a reference` error which would be particularly hard
to diagnose. One of the primary reasons for this failure is when the
`rootDir` option is configured---typically the case for libraries---
and a source file is imported using a relative import from an external
entry-point. This would normally report TS6059 for the invalid relative
import, but the crash prevents this error from being surfaced.

This commit refactors the reference emit logic to result in an explicit
`Failure` state with a reason as to why the failure occurred. This state
is then used to report a `FatalDiagnosticException`, preventing a hard
crash.

Closes #44414

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…ce file path (#44587)

Using `absoluteFromSourceFile` leverages the cache of the resolved
absolute path, instead of having to compute it each time.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…4587)

This commit fixes an issue with symlink handling in `MockFileSystem`,
where entries within a symlink would fail to resolve.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…44587)

The `NgtscCompilerHost` is implemented using the `FileSystem`
abstraction of the compiler, which is implemented for tests using an
in-memory `MockFileSystem`. If the in-memory filesystem contains
symlinks, then using `NgtscCompilerHost` would not reflect their
resolved real path. Instead, the TypeScript compiler would use its
default implementation based on the real filesystem, which is unaware of
the in-memory `MockFileSystem` setup.

This change does not currently address any issues, but is being fixed
as it prevented a reproduction scenario from behaving correctly.

PR Close #44587
atscott added a commit that referenced this issue Jan 6, 2022
…44587)

When building a library, the `rootDir` option is configured to ensure
that all source files are present within the entry-point that is being
build. This imposes an extra constraint on the reference emit logic,
which does not allow emitting a reference into a source file outside of
this `rootDir`.

During the generation of type-check blocks we used to make a best-effort
estimation of whether a type reference can be emitted into the
type-check file. This check was relaxed in #42492 to support emitting
more syntax forms and type references, but this change did not consider
the `rootDir` constraint that is present in library builds. As such, the
compiler might conclude that a type reference is eligible for emit into
the type-check file, whereas in practice this would cause a failure.

This commit changes the best-effort estimation into a "preflight"
reference emit that is fully accurate as to whether emitting a type
reference is possible.

Fixes #43624

PR Close #44587
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Feb 6, 2022

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.