ngtsc incremental correctness issues (language server) #39923
Closed
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
alxhub
added
the
area: language-service
alxhub
added
action: review
atscott
approved these changes
Dec 2, 2020
| @@ -375,12 +372,16 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop | |||
| for (const decl of ngModule.exports) { | |||
| // Attempt to resolve decl as an NgModule. | |||
| const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'export'); | |||
| if (importScope === undefined) { | |||
| if (importScope === 'invalid' || (importScope !== null && importScope.exported.isPoisoned)) { | |||
There was a problem hiding this comment.
nit: I know this name already existed, but I think this variable should be exportScope?
There was a problem hiding this comment.
You know, I think you might be right :)
alxhub
force-pushed
the
ngtsc/ls/correctness-issues
branch
4 times, most recently
from
804072c
to
8cafe80
Compare
Previously, if a trait's analysis step resulted in diagnostics, the trait would be considered "errored" and no further operations, including register, would be performed. Effectively, this meant that the compiler would pretend the 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 to a template error, the NgModule which declares the component would produce a diagnostic 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 compiler fails to associate a component and its module, then incremental builds will not 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 of issues 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 the right classes are re-analyzed when needed, instead of showing stale diagnostics. However, the dependency graph was not intended to serve this role, and could potentially be optimized in ways that would break this functionality. This commit removes the concept of an "errored" trait entirely from the trait system. Instead, analyzed and resolved traits have corresponding (and separate) diagnostics, in addition to potentially `null` analysis results. Analysis (but not resolution) diagnostics are carried forward during incremental build operations. Compilation (emit) is only performed when a trait reaches the resolved state with no diagnostics. This change is functionally different than before as the `register` step is now performed even in the presence of analysis errors, as long as analysis results are also produced. This fixes problem 1 above, and is part of the larger solution to problem 2.
To avoid overwhelming a user with secondary diagnostics that derive from a "root cause" error, the compiler has the notion of a "poisoned" NgModule. An NgModule becomes poisoned when its declaration contains semantic errors: declarations which are not components or pipes, imports which are not other NgModules, etc. An NgModule also becomes poisoned if it imports or exports another poisoned NgModule. Previously, the compiler tracked this poisoned status as an alternate state for each scope. Either a correct scope could be produced, or the entire scope would be set to a sentinel error value. This meant that the compiler would not track any information about a scope that was determined to be in error. This method presents several issues: 1. The compiler is unable to support the language service and return results when a component or its module scope is poisoned. This is fine for compilation, since diagnostics will be produced showing the error(s), but the language service needs to still work for incorrect code. 2. `getComponentScopes()` does not return components with a poisoned scope, which interferes with resource tracking of incremental builds. If the component isn't included in that list, then the NgModule for it will not have its dependencies properly tracked, and this can cause future incremental build steps to produce incorrect results. This commit changes the tracking of poisoned module scopes to use a flag on the scope itself, rather than a sentinel value that replaces the scope. This means that the scope itself will still be tracked, even if it contains semantic errors. A test is added to the language service which verifies that poisoned scopes can still be used in template type-checking.
Previously, if a component had an external template with a hard error, the compiler would "forget" the link between that component and its NgModule. Additionally, the NgModule would be marked as being in error, because the template issue would prevent the compiler from registering the component class as a component, so from the NgModule it would look like a declaration of a non-directive/pipe class. As a combined result, the next incremental step could fix the template error, but would not refresh diagnostics for the NgModule, leading to an incrementality issue. The various facets of this problem were fixed in prior commits. This commit adds a test verifying the above case works now as expected.
When the compiler is invoked via ngc or the Angular CLI, its APIs are used under the assumption that Angular analysis/diagnostics are only requested if the program has no TypeScript-level errors. A result of this assumption is that the incremental engine has not needed to resolve changes via its dependency graph when the program contained broken imports, since broken imports are a TypeScript error. The Angular Language Service for Ivy is using the compiler as a backend, and exercising its incremental compilation APIs without enforcing this assumption. As a result, the Language Service has run into issues where broken imports cause incremental compilation to fail and produce incorrect results. This commit introduces a mechanism within the compiler to keep track of files for which dependency analysis has failed, and to always treat such files as potentially affected by future incremental steps. This is tested via the Language Service infrastructure to ensure that the compiler is doing the right thing in the case of invalid imports.
alxhub
force-pushed
the
ngtsc/ls/correctness-issues
branch
from
8cafe80
to
32b9b56
Compare
petebacondarwin
added
compiler: language-server
and removed
compiler: language-server
labels
alxhub
added
action: merge
kyliau
approved these changes
Dec 3, 2020
mhevery
pushed a commit
that referenced
this pull request
Dec 3, 2020
To avoid overwhelming a user with secondary diagnostics that derive from a "root cause" error, the compiler has the notion of a "poisoned" NgModule. An NgModule becomes poisoned when its declaration contains semantic errors: declarations which are not components or pipes, imports which are not other NgModules, etc. An NgModule also becomes poisoned if it imports or exports another poisoned NgModule. Previously, the compiler tracked this poisoned status as an alternate state for each scope. Either a correct scope could be produced, or the entire scope would be set to a sentinel error value. This meant that the compiler would not track any information about a scope that was determined to be in error. This method presents several issues: 1. The compiler is unable to support the language service and return results when a component or its module scope is poisoned. This is fine for compilation, since diagnostics will be produced showing the error(s), but the language service needs to still work for incorrect code. 2. `getComponentScopes()` does not return components with a poisoned scope, which interferes with resource tracking of incremental builds. If the component isn't included in that list, then the NgModule for it will not have its dependencies properly tracked, and this can cause future incremental build steps to produce incorrect results. This commit changes the tracking of poisoned module scopes to use a flag on the scope itself, rather than a sentinel value that replaces the scope. This means that the scope itself will still be tracked, even if it contains semantic errors. A test is added to the language service which verifies that poisoned scopes can still be used in template type-checking. PR Close #39923
mhevery
pushed a commit
that referenced
this pull request
Dec 3, 2020
…39923) Previously, if a component had an external template with a hard error, the compiler would "forget" the link between that component and its NgModule. Additionally, the NgModule would be marked as being in error, because the template issue would prevent the compiler from registering the component class as a component, so from the NgModule it would look like a declaration of a non-directive/pipe class. As a combined result, the next incremental step could fix the template error, but would not refresh diagnostics for the NgModule, leading to an incrementality issue. The various facets of this problem were fixed in prior commits. This commit adds a test verifying the above case works now as expected. PR Close #39923
mhevery
pushed a commit
that referenced
this pull request
Dec 3, 2020
…rts (#39923) When the compiler is invoked via ngc or the Angular CLI, its APIs are used under the assumption that Angular analysis/diagnostics are only requested if the program has no TypeScript-level errors. A result of this assumption is that the incremental engine has not needed to resolve changes via its dependency graph when the program contained broken imports, since broken imports are a TypeScript error. The Angular Language Service for Ivy is using the compiler as a backend, and exercising its incremental compilation APIs without enforcing this assumption. As a result, the Language Service has run into issues where broken imports cause incremental compilation to fail and produce incorrect results. This commit introduces a mechanism within the compiler to keep track of files for which dependency analysis has failed, and to always treat such files as potentially affected by future incremental steps. This is tested via the Language Service infrastructure to ensure that the compiler is doing the right thing in the case of invalid imports. PR Close #39923
|
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.
A combination PR of several correctness fixes for LS integration with the compiler.