Skip to content

Commit

Permalink
New: Fixable Disable Directives (#78)
Browse files Browse the repository at this point in the history
* New: Fixable Disable Directives

* First two FAQs: autofix by default, opt out

* Clarified default CLI behavior

* Elaborated on fixes to account for remaining used rules

* Remaining -->

* Update designs/2021-fixable-disable-directives/design.md

Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>
  • Loading branch information
3 people committed May 20, 2021
1 parent 8ff6fec commit 101d00f
Showing 1 changed file with 136 additions and 0 deletions.
136 changes: 136 additions & 0 deletions designs/2021-fixable-disable-directives/design.md
@@ -0,0 +1,136 @@
- Repo: eslint/eslint
- Start Date: 2021-04-11
- RFC PR: https://github.com/eslint/rfcs/pull/78
- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg)

# Fixable Disable Directives

## Summary

Inline disable directives such as `/* eslint-disable */` that do not suppress any violations may be detected with the `--report-unused-disable-directives` CLI option, but there is no way to automatically remove those comments.
This RFC proposes adding removal of those directives to the `--fix` CLI option as a new [`--fix-type`](https://eslint.org/docs/user-guide/command-line-interface#-fix-type): _**`directive`**_.

## Motivation

Manually deleting comments from `--report-unused-disable-directives` is cumbersome, especially in large repositories with many directives.
Users would benefit from a quick way to fix these without having to manually map between CLI output lines and files.

## Detailed Design

Recapping the existing flow of code:

- When the `--fix-type` CLI option is specified, `options.fix` is patched to filter on the `rule.meta` of each fix's corresponding `ruleId`
- Unused disable directives are calculated by an `applyDirectives` function within the `applyDisableDirectives` function called in the linter's `_verifyWithoutProcessors`
- These problems have a `ruleId` of `null`
- `_verifyWithoutProcessors` is called within the call stack of each of the 1-10 passes in the linter's `verifyAndFix`

This RFC proposes making three changes:

- In the patched `options.fix`, consider a problem's meta type to be `"directive"` if its `ruleId` is `null`
- Pass the `SourceCode` being linted as a parameter to `applyDisableDirectives`
- Add a `fix` method to the unused directive problems returned by `applyDirectives` that uses the `SourceCode`

### Fix Behavior

Directives where at least one rule is still used will have only the unused rule names removed from their source text.

Directives where all >=1 rules are unused will use the `SourceCode` to compute:

- If they are the only non-whitespace on their line, delete that line
- Otherwise, delete just the comment and any now-unnecessary surrounding whitespace

#### Fix Behavior Examples

```diff
- /* eslint-disable */
```

```diff
- // eslint-disable-next-line -- related explanation
```

```diff
- // eslint-disable-next-line unused, used
+ // eslint-disable-next-line used
```

```diff
- before /* eslint-disable-next-line -- related explanation */ after
+ before after
```

```diff
- before // eslint-disable-next-line -- related explanation
+ before
```

```diff
// before
- // eslint-disable-next-line
// after
```

Multiline block comments are already not allowed to be disable directives. [eslint/eslint#10334](https://github.com/eslint/eslint/issues/10334)

## Documentation

- [Command Line Interface > Fixing Problems](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems)'s `--fix` documentation should mention the added directives fixing and the new `--fix-type`
- [Rules > Report Unused ESLint Disable Comments](https://eslint.org/docs/user-guide/configuring/rules#report-unused-eslint-disable-comments) should link to that documentation

## Drawbacks

Like any new feature, this flag will slightly increase the complexity and maintenance costs of ESLint core.

This RFC's implementation would lock in the name for a new `--fix-type` even though we only have one concrete use case for it.

## Backwards Compatibility Analysis

It is unlikely but not impossible that some `--fix` dependant users would rely on unused disable directives remaining in code.

Otherwise, this change is additive in behavior.

## Alternatives

- Applying directive fixes after the `verifyAndFix` passes
- Not ideal: deleting a comment could introduce new rule violations that would warrant running >=1 other pass
- Writing an external tool to apply these options
- Not ideal: the internal implementation is simple; external would have to deal with variant formatters, etc and suffer from the same introduced rule violations

## Open Questions

- Is `"directive"` a good name for the fix type?
- It feels like it could be too specific to be extended for other meta/configuration in the future.
- This RFC originally proposed `"meta"` but that goes too far in being overly vague.

## Help Needed

I'm looking forward to implementing this if approved! 🙌

## Frequently Asked Questions

> Will these be autofixed by default?
If `--fix` and `--report-unused-disable-directives` are both true, then yes.
There is no additional configuration that would need to be provided.

If `--fix` is true but `--report-unused-disable-directives` is not, there will be no behavior change from this RFC.
Unused directives would not add to reported problems and thus no new fixers would be added to them.

If `--fix` is not true but `--report-unused-disable-directives` is, there will be no observable change from this RFC for CLI clients.
Problems for unused directives will have a `fix` property attached but it will not be used without `--fix`.

> Can I opt out?
Yes.
If you are in the peculiar situation of needing to enable `--fix` and `--report-unused-disable-directives` _without_ fixing those directives _(why?)_, you can use `--fix-type` with all types except `directive`.

> How do the generated fixes compare to fixes from rules?
Problems reported by directive usage checking are joined with remaining rule violation problems in a single array.
This should allow directive fixing to seamlessly act similarly to rule fixing.

## Related Discussions

- [eslint/eslint#10334](https://github.com/eslint/eslint/issues/10334) - Report an error for eslint-disable-line comments that span multiple lines #1033)
- [eslint/eslint#9249](https://github.com/eslint/eslint/issues/9249) - Add CLI option to report unused eslint-disable directives
- [eslint/eslint#11815](https://github.com/eslint/eslint/issues/11815) - --report-unused-disable-directives should be autofixable

0 comments on commit 101d00f

Please sign in to comment.