From 530f173d8c9880cc04cb3d5b6bde2f4dd75eafb6 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Mon, 22 Jul 2019 14:42:02 -0700 Subject: [PATCH 1/5] New: Warn When Directives Use the Wrong Type of Comment --- .../README.md | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md diff --git a/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md b/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md new file mode 100644 index 00000000..ec24be59 --- /dev/null +++ b/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md @@ -0,0 +1,93 @@ +- Start Date: 2019-07-22 +- RFC PR: (leave this empty, to be filled in later) +- Authors: Jordan Eldredge + +# Warn When Directives Use the Wrong Type of Comment + +## Summary + +ESLint supports file level configuration via the use of [directives specified in code comments](https://eslint.org/docs/user-guide/configuring#using-configuration-comments). Some types of directives are only allowed in block level comments (`/* Block Level Comment */`): + +* `exported` +* `global(s)` +* `eslint-(en|dis)able` +* `eslint` + +Currently if a user adds one of these directives in an inline comment (`// Inline Comment`) it will have no effect and they will need to consult the documentation to figure out what they did wrong. + +I propose that if we detect one of these directives in an inline commment that we present the user with a lint error. + +## Motivation + +As a user of ESLint I occasionally wish to disable a rule, declare a valid global or add some other file level config. In doing so I often forget that some directives are only valid inside block level comments and add them as inline comments by mistake. When the comment fails to have its desired effect I consult the documentation to see what I did wrong. Did I missremember the directive syntax or name? On more than one occasion the problem has been that I used the wrong kind of comment. + +It would be nice if I could have been notifed of that mistake the moment I saved my file, rather than having to reread the docs. + +I would imagine this type of feedback would be even more helpful for new users who are using directives for the first time. + +## Detailed Design + +The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) already has some examples of reporting problems with directive comments via the `createLintingProblem` function. + +Currently `getDirectiveComments` works by first handling `eslint-disable-(next-)line` directives, which are comment type agnostic, and then only looking for the other types of directives if the comment type is `Block`. + +Instead, after handling the `eslint-disable-(next-)line` directives, we could check to see if we are in an inline comment. If we are, we could add a problem with something like: + +```JavaScript +problems.push(createLintingProblem({ + ruleId: null, + message: `The ${match[1]} directive is only allowed in block comments.`, + loc: comment.loc +})); +``` + +## Documentation + +I don't beleive that this would requrie any additional documentation. + +## Drawbacks + +I don't see any drawbacks to adding this feedback. + +## Backwards Compatibility Analysis + +Users may find that upon upgrading to the version of ESLint that contains this change, that they have new warnings which they might need to fix before upgrading. + +## Alternatives + +This could also be implemented as a userland ESLint rule. For example, here's a rule I wrote which warns when a user uses the `eslint-disable rule-name` directive in an inline comment. The main downside of having this rule live outside of ESLint is that it would have to exactly match ESLint's directive parsing behavior as well as its notion of which directives are permitted in which types of comments. + +```JavaScript +const eslintRuleDirective = /^\s*(eslint-(enable|disable)) .*$/; + +function isInvalid(comment) { + return comment.type === 'Line' && eslintRuleDirective.test(comment.value); +} + +module.exports = function rule(context) { + const sourceCode = context.getSourceCode(); + sourceCode + .getAllComments() + .filter(isInvalid) + .forEach(comment => { + const directive = comment.value.replace(eslintRuleDirective, '$1'); + context.report( + comment, + `ESLint expects \`${directive}\` directives to be used in block level comments.`, + ); + }); + return {}; +}; +``` + +## Open Questions + +Should this rule be fixable? I don't currently see any directive problems which offer fixes. + +## Help Needed + +I expect that I could make the pull request myself without help. + +## Related Discussions + +* [Warn when ESLint directives which expect to be used in block level comments are used in inline comments #12014](https://github.com/eslint/eslint/issues/12014) \ No newline at end of file From c759e4fdcf3a7a7c9148cc5b2c1976e4576a7e82 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Fri, 26 Jul 2019 10:14:09 -0700 Subject: [PATCH 2/5] Change proposal to Allow All Directives in Line Comment --- .../README.md | 84 +++++++++++++++++ .../README.md | 93 ------------------- 2 files changed, 84 insertions(+), 93 deletions(-) create mode 100644 designs/2019-allow-all-directives-in-line-comments/README.md delete mode 100644 designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md diff --git a/designs/2019-allow-all-directives-in-line-comments/README.md b/designs/2019-allow-all-directives-in-line-comments/README.md new file mode 100644 index 00000000..035cfaf0 --- /dev/null +++ b/designs/2019-allow-all-directives-in-line-comments/README.md @@ -0,0 +1,84 @@ +- Start Date: 2019-07-22 +- RFC PR: (leave this empty, to be filled in later) +- Authors: Jordan Eldredge + +# Allow All Directives in Line Comments + +## Summary + +ESLint currently only allows the following types in directives to be used inside of block comments (`/* Block Comment */`): + +* `exported` +* `global(s)` +* `eslint-(en|dis)able` +* `eslint` + +Currently if a user adds one of these directives in a line comment (`// Line Comment`) the directive will be silently ignored. Which can be very confusing. + +I propose that we allow all directives to work in either type of comment. + +## Motivation + +As a user of ESLint I occasionally wish to disable a rule, declare a valid global or add some other file level config. In doing so I often forget that these directives are only valid inside block level comments and add them as inline comments by mistake. When the comment fails to have its desired effect I consult the documentation to see what I did wrong. Did I missremember the directive syntax or name? On more than one occasion the problem has been that I used the wrong kind of comment. + +It would be nice if I didn't have to think about the comment type that I'm using. + +## Detailed Design + +The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) currently does a `if (comment.type === "Block")` check before it looks for some types of directives. + +I believe we could simply remove that check. + +## Documentation + +We would need to update the [configuration](https://eslint.org/docs/user-guide/configuring#using-configuration-comments) documentation to remove the places where it states that some types of directives must use block comments. We would replace each of those comments with a note that "Before version _x.x_ this directive only worked in block comments" so that users using an older version of ESLint could figure out why their line comments were ignored. + +## Drawbacks + +I don't see any drawbacks to this change. + +## Backwards Compatibility Analysis + + +### Latent Directives + +Users may have added directives to their code using line comments and then never noticed that they had no effect. With this change, they will now begin functioning again. In some cases this may be welcome, since the user's original intention would finally be respected. In other cases the code may have changed since the user added the latent directive. In that case this change may cause an unwanted directive to be enabled. + +Anecdotally, I did an audit of a large swath of our code base and discovered only two directives which were being ignored due to their comment type. In both cases, enabling would be the correct behavior. + +### Accidental Directives + +Users may also have written inline comments which were not intended as a directive, but parse as directives. I found one example in ESLint's own codebase: + +> `lineNumTokenBefore = 0; // global return at beginning of script` + +-- [newline-before-return.js:173](https://github.com/eslint/eslint/blob/02d7542cfd0c2e95c2222b1e9e38228f4c19df19/lib/rules/newline-before-return.js#L137) + +This would result in the following, somewhat confusing, error after upgrading to a version that contained this change: + +``` +/Users/jeldredge/projects/eslint/lib/rules/newline-before-return.js + 137:51 error 'return' is defined but never used no-unused-vars + 137:58 error 'at' is defined but never used no-unused-vars + 137:61 error 'beginning' is defined but never used no-unused-vars + 137:71 error 'of' is defined but never used no-unused-vars + 137:74 error 'script' is defined but never used no-unused-vars + +✖ 5 problems (5 errors, 0 warnings) +``` + +## Alternatives + +An earlier revision of this RFC suggested reporting an error/warning when a directive that is only supported in a bock comment was found in a line comment. + +## Open Questions + +As far as I am aware, there are no open questions. + +## Help Needed + +I expect that I could make the pull request myself without help. + +## Related Discussions + +* [Warn when ESLint directives which expect to be used in block level comments are used in inline comments #12014](https://github.com/eslint/eslint/issues/12014) \ No newline at end of file diff --git a/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md b/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md deleted file mode 100644 index ec24be59..00000000 --- a/designs/2019-warn-when-directives-use-the-wrong-type-of-comment/README.md +++ /dev/null @@ -1,93 +0,0 @@ -- Start Date: 2019-07-22 -- RFC PR: (leave this empty, to be filled in later) -- Authors: Jordan Eldredge - -# Warn When Directives Use the Wrong Type of Comment - -## Summary - -ESLint supports file level configuration via the use of [directives specified in code comments](https://eslint.org/docs/user-guide/configuring#using-configuration-comments). Some types of directives are only allowed in block level comments (`/* Block Level Comment */`): - -* `exported` -* `global(s)` -* `eslint-(en|dis)able` -* `eslint` - -Currently if a user adds one of these directives in an inline comment (`// Inline Comment`) it will have no effect and they will need to consult the documentation to figure out what they did wrong. - -I propose that if we detect one of these directives in an inline commment that we present the user with a lint error. - -## Motivation - -As a user of ESLint I occasionally wish to disable a rule, declare a valid global or add some other file level config. In doing so I often forget that some directives are only valid inside block level comments and add them as inline comments by mistake. When the comment fails to have its desired effect I consult the documentation to see what I did wrong. Did I missremember the directive syntax or name? On more than one occasion the problem has been that I used the wrong kind of comment. - -It would be nice if I could have been notifed of that mistake the moment I saved my file, rather than having to reread the docs. - -I would imagine this type of feedback would be even more helpful for new users who are using directives for the first time. - -## Detailed Design - -The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) already has some examples of reporting problems with directive comments via the `createLintingProblem` function. - -Currently `getDirectiveComments` works by first handling `eslint-disable-(next-)line` directives, which are comment type agnostic, and then only looking for the other types of directives if the comment type is `Block`. - -Instead, after handling the `eslint-disable-(next-)line` directives, we could check to see if we are in an inline comment. If we are, we could add a problem with something like: - -```JavaScript -problems.push(createLintingProblem({ - ruleId: null, - message: `The ${match[1]} directive is only allowed in block comments.`, - loc: comment.loc -})); -``` - -## Documentation - -I don't beleive that this would requrie any additional documentation. - -## Drawbacks - -I don't see any drawbacks to adding this feedback. - -## Backwards Compatibility Analysis - -Users may find that upon upgrading to the version of ESLint that contains this change, that they have new warnings which they might need to fix before upgrading. - -## Alternatives - -This could also be implemented as a userland ESLint rule. For example, here's a rule I wrote which warns when a user uses the `eslint-disable rule-name` directive in an inline comment. The main downside of having this rule live outside of ESLint is that it would have to exactly match ESLint's directive parsing behavior as well as its notion of which directives are permitted in which types of comments. - -```JavaScript -const eslintRuleDirective = /^\s*(eslint-(enable|disable)) .*$/; - -function isInvalid(comment) { - return comment.type === 'Line' && eslintRuleDirective.test(comment.value); -} - -module.exports = function rule(context) { - const sourceCode = context.getSourceCode(); - sourceCode - .getAllComments() - .filter(isInvalid) - .forEach(comment => { - const directive = comment.value.replace(eslintRuleDirective, '$1'); - context.report( - comment, - `ESLint expects \`${directive}\` directives to be used in block level comments.`, - ); - }); - return {}; -}; -``` - -## Open Questions - -Should this rule be fixable? I don't currently see any directive problems which offer fixes. - -## Help Needed - -I expect that I could make the pull request myself without help. - -## Related Discussions - -* [Warn when ESLint directives which expect to be used in block level comments are used in inline comments #12014](https://github.com/eslint/eslint/issues/12014) \ No newline at end of file From b72d4ef095610f2e2ba792a1c93daa1a50f60b68 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Fri, 4 Oct 2019 09:23:09 -0700 Subject: [PATCH 3/5] Add notes about the option of adding warnings in a minor release --- .../README.md | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/designs/2019-allow-all-directives-in-line-comments/README.md b/designs/2019-allow-all-directives-in-line-comments/README.md index 035cfaf0..f0aac1e9 100644 --- a/designs/2019-allow-all-directives-in-line-comments/README.md +++ b/designs/2019-allow-all-directives-in-line-comments/README.md @@ -25,10 +25,12 @@ It would be nice if I didn't have to think about the comment type that I'm using ## Detailed Design -The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) currently does a `if (comment.type === "Block")` check before it looks for some types of directives. +The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) currently does an `if (comment.type === "Block")` check before it looks for some types of directives. I believe we could simply remove that check. +For `/* eslint-env */` directives which, by necessity, are parsed out of the file before an AST is avaliable, the regular expression in [linter.js](https://github.com/eslint/eslint/blob/fb08b7c9d28bc68864eb940e26df274059228b6a/lib/linter/linter.js#L406) would need to be updated to allow it to match line comments. + ## Documentation We would need to update the [configuration](https://eslint.org/docs/user-guide/configuring#using-configuration-comments) documentation to remove the places where it states that some types of directives must use block comments. We would replace each of those comments with a note that "Before version _x.x_ this directive only worked in block comments" so that users using an older version of ESLint could figure out why their line comments were ignored. @@ -39,7 +41,6 @@ I don't see any drawbacks to this change. ## Backwards Compatibility Analysis - ### Latent Directives Users may have added directives to their code using line comments and then never noticed that they had no effect. With this change, they will now begin functioning again. In some cases this may be welcome, since the user's original intention would finally be respected. In other cases the code may have changed since the user added the latent directive. In that case this change may cause an unwanted directive to be enabled. @@ -69,7 +70,34 @@ This would result in the following, somewhat confusing, error after upgrading to ## Alternatives -An earlier revision of this RFC suggested reporting an error/warning when a directive that is only supported in a bock comment was found in a line comment. +An earlier revision of this RFC suggested reporting an error/warning when a directive that is only supported in a bock comment was found in a line comment. See the following section for a discussion of why this may be a worth doing in addition to the changes detailed above. + +## Optional Short Term Improvements + +Since code bases may contain line comments which were not intended as directives, but would be parsed as such after this change, (see [Accidental Directives](#accidental-directives) above) this change will be a breaking change and thus will need to wait for a major release. If the next major release is far off we could employ a short term fix of making line comment directives warnings which could be shipped in a minor release. + +This would have two benefits: + +1. In the time between when the minor release ships and when the next major release ships, user would get explicit feedback when they accidentally write a line comment directive when ESLint is expecting a block comment directive rather than having it silently fail. +2. Accidental directives would be surfaced as errors in the minor version which means that those who adopt the minor release before the next major release, and resolved all new warnings would have a smooth upgrade experience. + +### Implementation of Warnings + +If we decide to do the additional work of adding warnings in a minor version, here is how it could be implemented: + +The function `getDirectiveComments` in [`linter.js`](https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/linter/linter.js#L263) already has some examples of reporting problems with directive comments via the `createLintingProblem` function. + +Currently `getDirectiveComments` works by first handling `eslint-disable-(next-)line` directives, which are comment type agnostic, and then only looking for the other types of directives if the comment type is `Block`. + +Instead, after handling the `eslint-disable-(next-)line` directives, we could check to see if we are in an inline comment. If we are, we could add a problem with something like: + +```JavaScript +problems.push(createLintingProblem({ + ruleId: null, + message: `The ${match[1]} directive is only allowed in block comments.`, + loc: comment.loc +})); +``` ## Open Questions From cdb0de9dcf8e175b3143da5c795f77c8497f4f2a Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Fri, 4 Oct 2019 09:26:03 -0700 Subject: [PATCH 4/5] Reapply typo corrections suggested by @platinumazure I accidentally deleted these with a force push so reapplying them here. --- designs/2019-allow-all-directives-in-line-comments/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2019-allow-all-directives-in-line-comments/README.md b/designs/2019-allow-all-directives-in-line-comments/README.md index f0aac1e9..471c1960 100644 --- a/designs/2019-allow-all-directives-in-line-comments/README.md +++ b/designs/2019-allow-all-directives-in-line-comments/README.md @@ -19,7 +19,7 @@ I propose that we allow all directives to work in either type of comment. ## Motivation -As a user of ESLint I occasionally wish to disable a rule, declare a valid global or add some other file level config. In doing so I often forget that these directives are only valid inside block level comments and add them as inline comments by mistake. When the comment fails to have its desired effect I consult the documentation to see what I did wrong. Did I missremember the directive syntax or name? On more than one occasion the problem has been that I used the wrong kind of comment. +As a user of ESLint I occasionally wish to disable a rule, declare a valid global or add some other file level config. In doing so I often forget that these directives are only valid inside block level comments and add them as inline comments by mistake. When the comment fails to have its desired effect I consult the documentation to see what I did wrong. Did I remember the directive syntax or name incorrectly? On more than one occasion the problem has been that I used the wrong kind of comment. It would be nice if I didn't have to think about the comment type that I'm using. @@ -70,7 +70,7 @@ This would result in the following, somewhat confusing, error after upgrading to ## Alternatives -An earlier revision of this RFC suggested reporting an error/warning when a directive that is only supported in a bock comment was found in a line comment. See the following section for a discussion of why this may be a worth doing in addition to the changes detailed above. +An earlier revision of this RFC suggested reporting an error/warning when a directive that is only supported in a block comment was found in a line comment. See the following section for a discussion of why this may be a worth doing in addition to the changes detailed above. ## Optional Short Term Improvements From 466125076c80756a6a81c3efaee19cc51707f785 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Fri, 4 Oct 2019 20:55:53 -0700 Subject: [PATCH 5/5] Incorporate @ilyavolodin's suggestions Fix a typo and add an explanation about this being a breaking change. --- .../2019-allow-all-directives-in-line-comments/README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/designs/2019-allow-all-directives-in-line-comments/README.md b/designs/2019-allow-all-directives-in-line-comments/README.md index 471c1960..185f3691 100644 --- a/designs/2019-allow-all-directives-in-line-comments/README.md +++ b/designs/2019-allow-all-directives-in-line-comments/README.md @@ -6,7 +6,7 @@ ## Summary -ESLint currently only allows the following types in directives to be used inside of block comments (`/* Block Comment */`): +ESLint currently only allows the following types of directives to be used inside of block comments (`/* Block Comment */`): * `exported` * `global(s)` @@ -41,6 +41,10 @@ I don't see any drawbacks to this change. ## Backwards Compatibility Analysis +While this change is backwards compatible in that it does not change any existing API contract, there is a possiblity that user's code bases will contain [accidental directives](#accidental-directives) (see below) which could potentially mean that upgrading to a version that contained this change would result in lint errors when it did not before. + +Therefore, we will consider this change a _breaking change_. + ### Latent Directives Users may have added directives to their code using line comments and then never noticed that they had no effect. With this change, they will now begin functioning again. In some cases this may be welcome, since the user's original intention would finally be respected. In other cases the code may have changed since the user added the latent directive. In that case this change may cause an unwanted directive to be enabled.