fix(compiler-cli): report non-template diagnostics #40331
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
zarend
force-pushed
the
non-template-diagnostics
branch
4 times, most recently
from
8531795
to
03e2894
Compare
atscott
added
the
area: compiler
zarend
force-pushed
the
non-template-diagnostics
branch
2 times, most recently
from
0fc956b
to
697504c
Compare
zarend
added
the
action: review
alxhub
reviewed
Jan 9, 2021
alxhub
reviewed
Jan 9, 2021
alxhub
reviewed
Jan 9, 2021
zarend
force-pushed
the
non-template-diagnostics
branch
2 times, most recently
from
9999842
to
0d22a3c
Compare
zarend
commented
Jan 12, 2021
| if (file === undefined) { | ||
| return this.compiler.getDiagnostics(); | ||
| } | ||
| return this.compiler.getDiagnosticsForFile(file, OptimizeFor.WholeProgram); |
There was a problem hiding this comment.
@alxhub , I made this use WholeProgram optimization for two reasons.
- the previous behavior used WholeProgram, so it's better not to change it unless we have a good reason to
- I not sure how this will be used. If someone calls this in a loop one file at a time, we definitely want to use WholeProgram mode
zarend
commented
Jan 12, 2021
zarend
commented
Jan 12, 2021
| @@ -51,7 +51,7 @@ export class LanguageService { | |||
| const program = compiler.getNextProgram(); | |||
| const sourceFile = program.getSourceFile(fileName); | |||
| if (sourceFile) { | |||
| diagnostics.push(...ttc.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile)); | |||
| diagnostics.push(...compiler.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile)); | |||
There was a problem hiding this comment.
@alxhub , I made this use SingleFile optimization because that's what the previous behavior was, and I don't want to give up single file optimization and cause a performance regression.
If we wanted to call this with WholeProgram optimization, we have two options.
- manually ensure all shims are built before calling this, that way we use the memoized shims
- expose an optimizeFor parameter on this function.
There was a problem hiding this comment.
I think this is correct here too :)
zarend
force-pushed
the
non-template-diagnostics
branch
from
d6afab8
to
f986617
Compare
alxhub
approved these changes
Jan 13, 2021
atscott
approved these changes
Jan 13, 2021
kyliau
approved these changes
Jan 13, 2021
kyliau
added
action: cleanup
kyliau
added
the
area: language-service
Report non-template diagnotics when calling `getDiagnotics` function of the language service we only returned template diagnotics. This change causes it to return all diagnotics, not just diagnostics from the template type checker.
zarend
force-pushed
the
non-template-diagnostics
branch
from
f986617
to
94361c5
Compare
zarend
added
action: presubmit
|
presubmit is running... |
atscott
added
action: merge
clydin
pushed a commit
to angular/angular-cli
that referenced
this pull request
Jan 14, 2021
…gnostics` With this change we replace `getDiagnostics` with `getDiagnosticsForFile`. `getDiagnostics` no longer accept a parameter and is intended to be used instead of `getDiagnostics`. I used `OptimizeFor.WholeProgram` based on the comments in angular/angular#40331 (comment) which suggests that if you iterate through files one at a time the `WholeProgram` flag should be used. For more context see: angular/angular#40331 (cherry picked from commit 5fdc0a6)
clydin
pushed a commit
to angular/angular-cli
that referenced
this pull request
Jan 14, 2021
…gnostics` With this change we replace `getDiagnostics` with `getDiagnosticsForFile`. `getDiagnostics` no longer accept a parameter and is intended to be used instead of `getDiagnostics`. I used `OptimizeFor.WholeProgram` based on the comments in angular/angular#40331 (comment) which suggests that if you iterate through files one at a time the `WholeProgram` flag should be used. For more context see: angular/angular#40331
|
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.
Report non-template diagnostics when calling
getDiagnosticsfunction ofthe language service we only returned template diagnostics. This change
causes it to return all diagnostics, not just diagnostics from the
template type checker.
Why does this split getDiagnostics into two functions?
A. I split getDiagnostics into two functions. getDiagnostics for getting all diagnostics in the program, and getDiagnosticsForFile, which only gets diagnostics for the argument file. this is because it makes the implementation less complicated, and it aligns with our style guide – better to have two functions that do different things than one function that can do both things.