Support Sub Rule Suppression in TypeSpec Validation Tool

[wanlwanl]

Background

Validation rules for tspconfig.yaml are already added to TypeSpec validation tool to verify all parameters and options in tspconfig.yaml. But some RP may not want to add the some options for some emitter to get a special result, or just leave options for some emitters.

Intrduction

This PR adds the supports for sub-rule suppression in the TypeSpec validation tool.
Add sub-rules field to support suppress sub rules in some rule to gain finer control.
Sample suppressions.yaml:

- tool: TestTool
  path: foo
  rule: my-rule
  sub-rules:
    - my.option.a
    - my.option.b
  reason: test

TODO

Add doc for how to configure the suppressions.yaml in next PR.

[openapi-pipeline-app]

Next Steps to Merge

Next steps that must be taken to merge this PR:

• ❌ The required check named Protected Files has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

[openapi-pipeline-app]

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

[wanlwanl]

Hi @mikeharder , please note that I didn't add the wild card support sub-rules, because I don't want to encourage customer to suppress the whole emitter's options, I don't want to make it too easy at least, since our goal is to make all RPs configure all the options for all languages.

[mikeharder]

I added rules and sub-rules to suppressions in this PR:

#32902

Please adjust this PR accordingly. I reverted changes to the core rules engine and the rules other than sdk-tspconfig-validation, because they should only need minimal changes in this PR. Most of the changes should be contained to your rule. We may need to tweak the code in index.ts to distinguish between suppressions for "all of TSV" and "suppressions for rules and/or sub-rules".

[mikeharder]

Hi @mikeharder , please note that I didn't add the wild card support sub-rules, because I don't want to encourage customer to suppress the whole emitter's options, I don't want to make it too easy at least, since our goal is to make all RPs configure all the options for all languages.

Your rule can decide how to interpret the sub-rule strings in the suppression. Exact match, regex, etc.

[wanlwanl]

I added rules and sub-rules to suppressions in this PR:

#32902

Please adjust this PR accordingly. I reverted changes to the core rules engine and the rules other than sdk-tspconfig-validation, because they should only need minimal changes in this PR. Most of the changes should be contained to your rule. We may need to tweak the code in index.ts to distinguish between suppressions for "all of TSV" and "suppressions for rules and/or sub-rules".

• Reuse: Looks like it's not conflict to loading and extracting sub rules in index.ts, which is reverted. I think every rules should extract the sub rules in the same way. For how to comsume the sub rules, it should be in certain rules. So I only made loading and extracting part shared.
• Naming: If the goal of the sub-rule is to provide extra parameters to the rule, I think we should consider another naming, like parameters. sub-rule sounds like a fixed string, similar to legal terms that should be fixed, otherwise, it's misleading.
• Risk: change rule to rules may cause risks when sub-rules is used as extra parameters for other rules. e.g. rule A will skip when sub rule contains string skip, while skip may be field that rule B wants to verify, then when rule B verify skip field, rule A will always skip. So it's the responsibility to reviewer to always aware all the details of sub-rules for all the rules, sounds like it's easy to make trouble when reviewer/maintainer changes.

[mikeharder]

• Reuse: Looks like it's not conflict to loading and extracting sub rules in index.ts, which is reverted. I think every rules should extract the sub rules in the same way. For how to comsume the sub rules, it should be in certain rules. So I only made loading and extracting part shared.

Currently, the only rule in TSV that supports suppressions (rule or sub-rule) is SdkTspConfigValidationRule. So I think any changes related to suppressions should be limited to this rule. Nothing should need to change outside this rule. If and when other rules support suppressions, we will need to think about this more deeply, and we might move to ESLint by then anyway.

Naming: If the goal of the sub-rule is to provide extra parameters to the rule, I think we should consider another naming, like parameters. sub-rule sounds like a fixed string, similar to legal terms that should be fixed, otherwise, it's misleading.

I don't understand your objection to the name sub-rule, but I am open to changing the name. Between sub-rule and parameters, I prefer the former, since I think it better matches what we are trying to describe: "a smaller rule within a larger rule".

Risk: change rule to rules may cause risks when sub-rules is used as extra parameters for other rules. e.g. rule A will skip when sub rule contains string skip, while skip may be field that rule B wants to verify, then when rule B verify skip field, rule A will always skip. So it's the responsibility to reviewer to always aware all the details of sub-rules for all the rules, sounds like it's easy to make trouble when reviewer/maintainer changes.

I want to make the core suppressions engine as flexible as possible, while also being as simple as possible. The suppressions engine is used by multiple checks, not just TSV. Allowing a user to suppress multiple rules in a single statement is very useful, and IMO outweighs the risk of someone "accidentally" suppressing more than they wanted.

[wanlwanl]

• Reuse: Looks like it's not conflict to loading and extracting sub rules in index.ts, which is reverted. I think every rules should extract the sub rules in the same way. For how to comsume the sub rules, it should be in certain rules. So I only made loading and extracting part shared.

Currently, the only rule in TSV that supports suppressions (rule or sub-rule) is SdkTspConfigValidationRule. So I think any changes related to suppressions should be limited to this rule. Nothing should need to change outside this rule. If and when other rules support suppressions, we will need to think about this more deeply, and we might move to ESLint by then anyway.

Naming: If the goal of the sub-rule is to provide extra parameters to the rule, I think we should consider another naming, like parameters. sub-rule sounds like a fixed string, similar to legal terms that should be fixed, otherwise, it's misleading.

I don't understand your objection to the name sub-rule, but I am open to changing the name. Between sub-rule and parameters, I prefer the former, since I think it better matches what we are trying to describe: "a smaller rule within a larger rule".

Risk: change rule to rules may cause risks when sub-rules is used as extra parameters for other rules. e.g. rule A will skip when sub rule contains string skip, while skip may be field that rule B wants to verify, then when rule B verify skip field, rule A will always skip. So it's the responsibility to reviewer to always aware all the details of sub-rules for all the rules, sounds like it's easy to make trouble when reviewer/maintainer changes.

I want to make the core suppressions engine as flexible as possible, while also being as simple as possible. The suppressions engine is used by multiple checks, not just TSV. Allowing a user to suppress multiple rules in a single statement is very useful, and IMO outweighs the risk of someone "accidentally" suppressing more than they wanted.

Confirmed that this is what the reviewer wants, and I will continue the implementation based on the existing design.

[wanlwanl]

@mikeharder updated, please review it

[mikeharder]

@wanlwanl: I updated the PR to only suppress SdkTspConfigValidation, and simplify the implementation and tests.

I removed the code in index.ts that suppressed at the rule level. If you want to allow spec authors to suppress your rule entirely (not just sub-rules within the rule), you might need to add this code back into your rule. As-is, I think your rule only supports sub-rule level suppressions.

But, this might actually be what you want, if spec authors should never disable your entire rule (only sub-rules).