Skip to content

Extended template diagnostics configuration#44391

Closed
dgp1130 wants to merge 7 commits intoangular:masterfrom
dgp1130:etd-config
Closed

Extended template diagnostics configuration#44391
dgp1130 wants to merge 7 commits intoangular:masterfrom
dgp1130:etd-config

Conversation

@dgp1130
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 commented Dec 7, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Refactor

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 _extendedTemplateDiagnostics option 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, or suppress. error will fail the compilation if the diagnostic is emitted, while suppress will 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 strictTemplates is 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 ExtendedTmplateDiagnosticName enum to act as a centralized list of all possible diagnostic names which can be used in the configuration. It also introduced a TemplateCheckFactory concept 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?

  • No

@dgp1130 dgp1130 added the target: patch This PR is targeted for the next patch release label Dec 7, 2021
@dgp1130 dgp1130 requested review from alxhub and atscott and removed request for atscott December 7, 2021 08:07
@dgp1130 dgp1130 force-pushed the etd-config branch 2 times, most recently from 6d64cdd to e61bfb3 Compare December 11, 2021 00:54
@dylhunn dylhunn added the area: core Issues related to the framework runtime label Jan 4, 2022
@ngbot ngbot bot added this to the Backlog milestone Jan 4, 2022
@dgp1130 dgp1130 requested a review from atscott January 4, 2022 19:09
@dgp1130 dgp1130 force-pushed the etd-config branch 3 times, most recently from 9c8b14c to 3a3cc4f Compare January 5, 2022 04:48
Copy link
Copy Markdown
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍪

Just the one comment about docs.

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from atscott January 6, 2022 18:57
Copy link
Copy Markdown
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

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.
Copy link
Copy Markdown
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

…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.
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label Jan 11, 2022
@atscott
Copy link
Copy Markdown
Contributor

atscott commented Jan 11, 2022

This PR was merged into the repository by commit 83e6db4.

@atscott atscott closed this in ed74e3a Jan 11, 2022
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
Refs #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.

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
Refs #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.

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
@dgp1130 dgp1130 deleted the etd-config branch January 11, 2022 22:49
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants