Extended template diagnostics configuration#44391
Closed
dgp1130 wants to merge 7 commits intoangular:masterfrom
Closed
Extended template diagnostics configuration#44391dgp1130 wants to merge 7 commits intoangular:masterfrom
dgp1130 wants to merge 7 commits intoangular:masterfrom
Conversation
6d64cdd to
e61bfb3
Compare
dgp1130
commented
Dec 11, 2021
packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
Outdated
Show resolved
Hide resolved
atscott
approved these changes
Jan 5, 2022
packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts
Outdated
Show resolved
Hide resolved
9c8b14c to
3a3cc4f
Compare
dgp1130
commented
Jan 5, 2022
packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
Outdated
Show resolved
Hide resolved
jessicajaniuk
approved these changes
Jan 6, 2022
Contributor
jessicajaniuk
left a comment
There was a problem hiding this comment.
LGTM 🍪
Just the one comment about docs.
reviewed-for: public-api
atscott
approved these changes
Jan 6, 2022
Contributor
atscott
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
Closed
5 tasks
08d93cf to
2f9551c
Compare
alxhub
approved these changes
Jan 10, 2022
packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
Outdated
Show resolved
Hide resolved
...es/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts
Outdated
Show resolved
Hide resolved
Refs angular#42966. This is a static array of all the 1P extended template diagnostic factories built into the Angular compiler directly. It provides an encapsulated list of all diagnostics rather than requiring users to manually list each one individually.
…extended template diagnostics Refs angular#42966. This includes a mapping of extended template diagnostics to their associated diagnostic category. It is generated from the list of diagnostic names, so the list should always be implicitly kept up to date. Usage looks like: ```json { "angularCompilerOptions": { "extendedDiagnostics": { "checks": { "invalidBananaInBox": "error", "nullishCoalescingNotNullable": "suppress" } } } } ```
…ing warnings from extended template diagnostics Refs angular#42966. This updates `TemplateContext` to include a new `makeTemplateDiagnostic()` function which automatically uses the correct diagnostic category for that check. This makes sure that each diagnostic is emitted with the correct category. It also implicitly passes some known values like `component` and `code` to make the extended template diagnostics a little simpler. Diagnostics which are suppressed are never instantiated at all, which acts as a slight performance optimization since any emitted diagnostics would be ignored anyways. Unfortunately, diagnostics still have access to `ctx.templateTypeChecker.makeTemplateDiagnostic()` to manually create diagnostics with a different category. Both banana in box and nullish coalescing checks include tests to make sure they respect a manually configured category. This convention should hopefully give a reasonable certainty that new diagnostics will use the correct reporting function, even if that is not strictly enforced.
Refs angular#42966. The `defaultCategory` option is used for any extended template diagnostics which do not have any specific category specified for them. It defaults to `warning`, since that is the most common behavior expected for users. This provides an easy way for users to promote all diagnostics to errors or suppress all diagnostics.
…cs configuration Refs angular#42966. This validates the `tsconfig.json` options for extended template diagnostics. It verifies: * `strictTemplates` must be enabled if `extendedDiagnostics` have any explicit configuration. * `extendedDiagnostics.defaultCategory` must be a valid `DiagnosticCategoryLabel`. * `extendedDiagnostics.checks` keys must all be real diagnostics. * `extendedDiagnostics.checks` values must all be valid `DiagnosticCategoryLabel`s. These include new error codes, each of which prints out the exact property that was the issue and what the available options are to fix them. It does disallow the config: ```json { "angularCompilerOptions": { "strictTemplates": false, "extendedDiagnostics": { "defaultCategory": "suppress" } } } ``` Such a configuration is technically valid and could be executed, but will be rejected by this verification logic. There isn't much reason to ever do this, since users could just drop `extendedDiagnostics` altogether and get the intended effect. This is unlikely to be a significant issue for users, so it is considered invalid for now to keep the implementation simple.
Contributor
|
This PR was merged into the repository by commit 83e6db4. |
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…e checker (#44391) Refs #42966. This moves extended template check factory invocations into the checker itself, where it can provide a more consistent API contract. Factories are called with compiler options and may return a `TemplateCheck` or `undefined` if the current options do not support that check. This allows `nullishCoalescingNotNullable` to disable itself when `strictNullChecks` is disabled without throwing errors. This gives extended template diagnostics a stronger abstraction model to define their behavior with. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…extended template diagnostics (#44391) Refs #42966. This includes a mapping of extended template diagnostics to their associated diagnostic category. It is generated from the list of diagnostic names, so the list should always be implicitly kept up to date. Usage looks like: ```json { "angularCompilerOptions": { "extendedDiagnostics": { "checks": { "invalidBananaInBox": "error", "nullishCoalescingNotNullable": "suppress" } } } } ``` PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…ing warnings from extended template diagnostics (#44391) Refs #42966. This updates `TemplateContext` to include a new `makeTemplateDiagnostic()` function which automatically uses the correct diagnostic category for that check. This makes sure that each diagnostic is emitted with the correct category. It also implicitly passes some known values like `component` and `code` to make the extended template diagnostics a little simpler. Diagnostics which are suppressed are never instantiated at all, which acts as a slight performance optimization since any emitted diagnostics would be ignored anyways. Unfortunately, diagnostics still have access to `ctx.templateTypeChecker.makeTemplateDiagnostic()` to manually create diagnostics with a different category. Both banana in box and nullish coalescing checks include tests to make sure they respect a manually configured category. This convention should hopefully give a reasonable certainty that new diagnostics will use the correct reporting function, even if that is not strictly enforced. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
Refs #42966. The `defaultCategory` option is used for any extended template diagnostics which do not have any specific category specified for them. It defaults to `warning`, since that is the most common behavior expected for users. This provides an easy way for users to promote all diagnostics to errors or suppress all diagnostics. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…cs configuration (#44391) Refs #42966. This validates the `tsconfig.json` options for extended template diagnostics. It verifies: * `strictTemplates` must be enabled if `extendedDiagnostics` have any explicit configuration. * `extendedDiagnostics.defaultCategory` must be a valid `DiagnosticCategoryLabel`. * `extendedDiagnostics.checks` keys must all be real diagnostics. * `extendedDiagnostics.checks` values must all be valid `DiagnosticCategoryLabel`s. These include new error codes, each of which prints out the exact property that was the issue and what the available options are to fix them. It does disallow the config: ```json { "angularCompilerOptions": { "strictTemplates": false, "extendedDiagnostics": { "defaultCategory": "suppress" } } } ``` Such a configuration is technically valid and could be executed, but will be rejected by this verification logic. There isn't much reason to ever do this, since users could just drop `extendedDiagnostics` altogether and get the intended effect. This is unlikely to be a significant issue for users, so it is considered invalid for now to keep the implementation simple. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…plateCheckFactory` (#44391) Refs #42966. The enum of extended template diagnostic names allows a global registry of first-party diagnostics with a developer-friendly string name which can be used for configuration. This name is used in the new `TemplateCheckFactory` to bind the name to a particular `ErrorCode` and make both available *before* constructing the actual template check, which is necessary to configure it appropriately. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…e checker (#44391) Refs #42966. This moves extended template check factory invocations into the checker itself, where it can provide a more consistent API contract. Factories are called with compiler options and may return a `TemplateCheck` or `undefined` if the current options do not support that check. This allows `nullishCoalescingNotNullable` to disable itself when `strictNullChecks` is disabled without throwing errors. This gives extended template diagnostics a stronger abstraction model to define their behavior with. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…extended template diagnostics (#44391) Refs #42966. This includes a mapping of extended template diagnostics to their associated diagnostic category. It is generated from the list of diagnostic names, so the list should always be implicitly kept up to date. Usage looks like: ```json { "angularCompilerOptions": { "extendedDiagnostics": { "checks": { "invalidBananaInBox": "error", "nullishCoalescingNotNullable": "suppress" } } } } ``` PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…ing warnings from extended template diagnostics (#44391) Refs #42966. This updates `TemplateContext` to include a new `makeTemplateDiagnostic()` function which automatically uses the correct diagnostic category for that check. This makes sure that each diagnostic is emitted with the correct category. It also implicitly passes some known values like `component` and `code` to make the extended template diagnostics a little simpler. Diagnostics which are suppressed are never instantiated at all, which acts as a slight performance optimization since any emitted diagnostics would be ignored anyways. Unfortunately, diagnostics still have access to `ctx.templateTypeChecker.makeTemplateDiagnostic()` to manually create diagnostics with a different category. Both banana in box and nullish coalescing checks include tests to make sure they respect a manually configured category. This convention should hopefully give a reasonable certainty that new diagnostics will use the correct reporting function, even if that is not strictly enforced. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
Refs #42966. The `defaultCategory` option is used for any extended template diagnostics which do not have any specific category specified for them. It defaults to `warning`, since that is the most common behavior expected for users. This provides an easy way for users to promote all diagnostics to errors or suppress all diagnostics. PR Close #44391
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2022
…cs configuration (#44391) Refs #42966. This validates the `tsconfig.json` options for extended template diagnostics. It verifies: * `strictTemplates` must be enabled if `extendedDiagnostics` have any explicit configuration. * `extendedDiagnostics.defaultCategory` must be a valid `DiagnosticCategoryLabel`. * `extendedDiagnostics.checks` keys must all be real diagnostics. * `extendedDiagnostics.checks` values must all be valid `DiagnosticCategoryLabel`s. These include new error codes, each of which prints out the exact property that was the issue and what the available options are to fix them. It does disallow the config: ```json { "angularCompilerOptions": { "strictTemplates": false, "extendedDiagnostics": { "defaultCategory": "suppress" } } } ``` Such a configuration is technically valid and could be executed, but will be rejected by this verification logic. There isn't much reason to ever do this, since users could just drop `extendedDiagnostics` altogether and get the intended effect. This is unlikely to be a significant issue for users, so it is considered invalid for now to keep the implementation simple. PR Close #44391
|
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. |
This file contains hidden or 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
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.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number: #42966
What is the new behavior?
Implements configuration support for extended template diagnostics. The engine now reads from compiler options to choose what diagnostics are enabled and what their categories are. This is still protected by the experimental
_extendedTemplateDiagnosticsoption so we can merge it to patch. A follow-up PR will enable this specifically for the next minor release.The configuration looks like:
{ "angularCompilerOptions": { "extendedDiagnostics": { // The categories to use for specific diagnostics. "checks": { // Maps check name to its category. "invalidBananaInBox": "suppress" }, // The category to use for any diagnostics not listed in `checks` above. "defaultCategory": "error" } } }Allowed categories for a diagnostic are
warning(default),error, orsuppress.errorwill fail the compilation if the diagnostic is emitted, whilesuppresswill ignore the diagnostic altogether. As an optimization, ignored diagnostics are not executed at all to avoid wasting CPU resources.The configuration is validated and will throw errors with specific codes for simple mistakes, such as a typo in a check name or the diagnostic category. Extended template diagnostics also require that
strictTemplatesis enabled, since they require much of the same type information and would be fairly useless without that knowledge.The implementation required some refactorings, adding a new
ExtendedTmplateDiagnosticNameenum to act as a centralized list of all possible diagnostic names which can be used in the configuration. It also introduced aTemplateCheckFactoryconcept which binds the error code and diagnostic name together in a way that is usable before constructing the template check. This allows us to skip suppressed checks by not even constructing them.Does this PR introduce a breaking change?