New: Allow All Directives in Line Comments

[captbaritone]

Note: This PR was previously titled "Warn When Directives Use the Wrong Type of Comment" but has evolved into a different proposal

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.

Related Issues

• Warn when ESLint directives which expect to be used in block level comments are used in inline comments #12014

[jsf-clabot]

All committers have signed the CLA.

[ljharb]

Could perhaps, instead of a warning (which is better than silent failure), it Just Work? Why does the comment style matter?

[captbaritone]

Great question, I’m afraid I don’t fully know. I assumed it was to ensure those more serious directives were given a weightier visual appearance, but @kaicataldo mentioned in eslint/eslint#12014 that it might be about enforcing some kind of granularity. Any idea who would know what the intention behind this limitation was?

[captbaritone]

I went searching through the Git and GitHub histories and tried to see if I could find any clues as to why we make a distinction between inline comments and block comments. I didn't find anything too concrete, but my instinct tells me that defining globals and enableing/disabling rules per-file were copied from JSHint which used block comments for this purpose.

I didn't see any good reason to leave this convention in place, and I saw a number of good reasons to get rid of it.

I would say @platinumazure and @ilyavolodin seem likely to have context on this.

Here are some interesting things I found, in chronological order:

Add no-undef (fixes #6) #164

Jul 28, 2013

This PR adds the /* global */ syntax. The requirement that comments be block level is not questioned in the PR, but I get the impression that the choice was borrowed from JSHint/JSLint.

Disabling/enabling rules through comments #320

Oct 3, 2013

In a comment nzakas questions why only block comments are supported.

@ilyavolodin cites a convention used in JSHint that block comments are for file-level configuration and inline are used for per-line config.

nzakas replies (emphasis mine):

Hmm okay. I'm not sure I want to emulate the JSHint behavior if this is true, it seems like block for global and non-block for local is too implicit to have meaning.

But agrees to punt.

New: Adding Support for "// eslint-disable-line rule" style comments #1840

Feb 16, 2015

In a comment nzakas suggests these rules be required to be inline. The PR author pushes back citing consistency and stating that all the other comments use block style. They eventually opt for requiring that eslint-disable-line use an inline comment.

This is where we start to have two code paths, one for inline and one for block level.

Docs: clarify usage of inline disable comments (fixes #6335) #6347

Jun 8, 2016

@kaicataldo updates the docs to clarify which comment directives require block level comments. In the related issue @platinumazure comments:

Yes, we very intentionally allow certain inline config comments to be line-only, and others are block-only. We could probably improve the documentation on that front.

But does not provide any context as to why.

[kaicataldo]

I'm interested to hear from some other team members who might have more context as to why we limit certain inline config comments to block comments, but I'm also unsure why we can't support both. Seems like that would be the best developer experience.

[ilyavolodin]

The guess that globals were added as block level comments to comply with JSHint is correct. The idea back then was that JSHint is a lot more popular, and a lot of people probably already have comments in their files declaring globals. If they wanted to switch to ESLint they would have to create a new configuration file, but at least, they wouldn't have to go file by file to change global variable declarations for linter.
I'm not opposed to supporting both styles of comments for all directives, but I do think that directives that deal with enabling/disabling things probably make more sense as inline comments vs block (disable next line, for example).

[kaicataldo]

That's right, I'm remembering the discussion around allowing block comments for eslint-disable-line and eslint-disable-next-line comments now. The issue we ran into was with how multiline block comments would be treated - i.e. how do you know which line should be disabled? As far as I can tell, this isn't an issue with other comments that aren't based on their line location.

[ljharb]

I'd assume the next line after the comment block ends or the line on which the comment block ends, and I'm not clear on what other possibilities there'd be?

[not-an-aardvark]

We already support block eslint-disable-[next-]line comments and report an error if the comment spans multiple lines. Changing all directives to support either line or block comments seems fine to me.

[captbaritone]

Okay. It seems like the idea of allowing any directive to exist in any type of comment has no immediate objections. Should I edit this RFC to reflect this new plan of attack, or open a new one?

[platinumazure]

I think editing this RFC should be fine.

[captbaritone]

I've updated the RFC to reflect the new plan of attack.

Note that I've switched to using "line comments" to refer to // comment, since this matches the AST syntax and I have no idea if "inline comment" is a clear name.

[mysticatea]

How about /* eslint-env */ comments? It's on lib/linter/linter.js#L413 because ESLint must find it before parsing.

[captbaritone]

@mysticatea I don't see any reason we couldn't extend that regex to support Line comments. If nobody objects, I'll add eslint-env to the RFC as well.

[platinumazure]

Two minor potential typos.

I'm also a bit concerned about the accidental directive business. I'm wondering if we should consider requiring that global comments, if they are line comments, must be the only thing on the line... or some other approach to reduce the chance of an accidental directive. Not sure what the best approach is here.

[ljharb]

I would be shocked to discover a single instance in any code anywhere of a directive-like comment that was not intended to be a directive.

[platinumazure]

@ljharb An example in ESLint's own codebase was identified in the RFC. The comment was talking about the globalReturn parser option but the comment started with // global return.

[ljharb]

@platinumazure i guess for globals comments, since global is allowed (even though i only ever use globals), but what about every other kind of directive?

[platinumazure]

but what about every other kind of directive?

I can't think of any examples of those, outside of maybe "eslint" (e.g., a comment // eslint is being silly here and we interpret that as trying to set an is rule).

[captbaritone]

@platinumazure I like the idea of trying to limit what we interpret as directives in order to reduce the likelihood of accidentally over-matching. However, I don't think forcing the directive to be alone on a line will work.

I'm guessing your hesitancy was due to these same concerns, but I'll spell them out just in case:

• It's very common (and useful) to use the rest of the line of the directive to explain the "why" behind the directive.
• Ignoring malformed directives, as opposed to matching them and triggering an error, can lead to the same type of bad developer experience that this RFC is designed to mitigate. If a user misunderstands the subtle details of how to construct a directive, it simply doesn't work rather than resulting in an error message which can help them discover their error.

Another option would be to write an ESlint rule which could detect these latent/accidental directives so that users could explicitly handle them before upgrading. While a bit cute, this solution is probably not great because it asks quite a lot of the person performing the upgrade.

1. Install this random ESLint rule
2. Resolve/address all issues
3. Remove the rule
4. Upgrade

I guess another option would be to go back to the original version of this RFC: Warn when we detect inline directives. And get that merged into a minor version and then wait some time before merging the change to make them allowed.

Honestly, that seems like a bit much given the low likelihood of these types of comments.

[platinumazure]

I've created this TSC meeting agenda item to try to resolve the semver question around warnings.

[platinumazure]

The TSC met yesterday and discussed this, and we agree, in principle, that adding warnings to improve UX for at least this use case could be allowable as a semver-minor change. Apologies for the delay in discussing this.

I've been tasked with creating an issue in eslint/eslint to discuss exactly how we want to do it, so we still have a few things to figure out. But in principle, we should be able to convert this RFC back to warning on line comments (if desired) and getting it out in semver-minor change.

My proposal is roughly going to be this:

1. Short-term, no technical changes, just process changes. On a case-by-case basis, TSC can allow specific exceptional cases where new warnings (and no errors) are generated and we will release semver-minor. These exceptional cases will never involve rules giving more warnings-- that will always be semver-minor if a bug fix only, or semver-major if not a bug fix.
2. Long-term, we will create a new property on the lint result object to contain configuration warnings or errors (or, more generally, warnings/errors that apply to the whole lint run rather than individual files/rules), and configuration warnings will never count against the --max-warnings CLI flag. During major releases, we would migrate appropriate file-level warnings into this property as necessary.

Assuming the TSC approves the process change described in point 1 above, we would then exercise this process to allow this RFC to move forward (hopefully).

I understand this has been a slow and frustrating process and I greatly appreciate your patience with us as we work through this. Thank you @captbaritone for your continued willingness to work with us on this RFC!

[platinumazure]

Earlier, I had proposed that another option might be to create a new CLI option or supplemental tool which lets users opt in to checking for these. This might be the best solution if we want to allow users to continue to write real comments (e.g., // eslint is buggy here immediately before an eslint-disable-next-line comment), but still give users a way to potentially find unexpectedly not-working ESLint comments.

To me, this completely bypasses the discussion about adding warnings and semver-major/-minor.

Thoughts?

[btmills]

We discussed this in today's TSC meeting as a potential breaking change for the v7.0.0 release. Since we have the option of a process change that would allow us to include this in a semver-minor release, we'd prefer to do that, and we don't have to hurry to get this in as part of the v7.0.0 release. More details on the process change can be found in eslint/eslint#12650.

[glen-84]

Recent comments seem to imply that single-line comment directives will not be supported (they will only result in a warning)?

Single-line comment directives are supported in stylelint and PHP_CodeSniffer, so please consider supporting them in ESLint.

I don't see why this would be a breaking change.

[nzakas]

@eslint/eslint-tsc it looks like we lost track of this RFC. How do we want to proceed?

[mdjermanovic]

After reading the RFC document and all discussions, I'm not sure that I understand what's the final proposal.

• Warn on directive-like line comments in a minor version, and treat them as real directives in a major version?
• Warn on directive-like line comments in a major version, and treat them as real directives in a subsequent major version?
• Just warn on directive-like line comments in a minor version?
• Just warn on directive-like line comments in a major version?
• Just treat them as real directives in a major version?

[nzakas]

@glen-84 this is off topic. If you’d like to discuss further, please create a new discussion. Let’s keep these comments targeted at the RFC.

[nzakas]

@mdjermanovic the RFC is proposing to introduce warnings in a minor version and then allow all directives in both block and line comments in a major version. The intent is to warn people about comments that look like directives but do nothing before we actually make them do something.

The warnings part is considered optional, though, so we could just skip directly to making directives work in all comments in a major version.

[glen-84]

@glen-84 this is off topic. If you’d like to discuss further, please create a new discussion. Let’s keep these comments targeted at the RFC.

Is it off topic? It's listed in the RFC under Accidental Directives.

By allowing global (for example) in line comments, it may cause issues with existing or even new code. That is why I was suggesting moving to a prefixed version of these directives.

Anyway, I apologize for the noise. I'll hide my previous messages.

[nzakas]

I understand your point, it’s just moving to prefixed directives is beyond the scope of this RFC at this point. It’s already in the final commenting period, during which we are mostly making a go/no-go decision.

[nzakas]

Okay, we like the direction, so merging. @captbaritone our apologies for losing track of this. We don’t have any excuse for letting this slip beneath our radar for so long.