no-warning-comments does not work with multiline comments

[andreyrd]

I'm going to report using the online demo rather than my own project, so that anyone else can reproduce this bug as well. It happens in the online demo. https://eslint.org/demo

Direct link to the demo

• ESLint Version: 4.16.0
• Node Version: N/A
• npm Version: N/A

What parser (default, Babel-ESLint, etc.) are you using?
Default (unless the online demo uses something else?)

Please show your full configuration:
I checked no-warning-comments in the demo and downloaded the JSON file.

Configuration

{
    "parserOptions": {
        "ecmaVersion": 5,
        "sourceType": "script",
        "ecmaFeatures": {}
    },
    "rules": {
        "constructor-super": 2,
        "no-case-declarations": 2,
        "no-class-assign": 2,
        "no-compare-neg-zero": 2,
        "no-cond-assign": 2,
        "no-console": 2,
        "no-const-assign": 2,
        "no-constant-condition": 2,
        "no-control-regex": 2,
        "no-debugger": 2,
        "no-delete-var": 2,
        "no-dupe-args": 2,
        "no-dupe-class-members": 2,
        "no-dupe-keys": 2,
        "no-duplicate-case": 2,
        "no-empty-character-class": 2,
        "no-empty-pattern": 2,
        "no-empty": 2,
        "no-ex-assign": 2,
        "no-extra-boolean-cast": 2,
        "no-extra-semi": 2,
        "no-fallthrough": 2,
        "no-func-assign": 2,
        "no-global-assign": 2,
        "no-inner-declarations": 2,
        "no-invalid-regexp": 2,
        "no-irregular-whitespace": 2,
        "no-mixed-spaces-and-tabs": 2,
        "no-new-symbol": 2,
        "no-obj-calls": 2,
        "no-octal": 2,
        "no-redeclare": 2,
        "no-regex-spaces": 2,
        "no-self-assign": 2,
        "no-sparse-arrays": 2,
        "no-this-before-super": 2,
        "no-undef": 2,
        "no-unexpected-multiline": 2,
        "no-unreachable": 2,
        "no-unsafe-finally": 2,
        "no-unsafe-negation": 2,
        "no-unused-labels": 2,
        "no-unused-vars": 2,
        "no-useless-escape": 2,
        "require-yield": 2,
        "use-isnan": 2,
        "valid-typeof": 2,
        "no-warning-comments": 2
    },
    "env": {}
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

/**
 * todo
 */

/*
 * todo
 */

// todo

var foo = bar;

Run it in the online demo.

What did you expect to happen?
The sections

/*
 * todo
 */

and

/**
 * todo
 */

should be flagged for no-warning-comments.

What actually happened? Please include the actual, raw output from ESLint.
Only

// todo

is flagged for no-warning-comments.

[mysticatea]

Thank you for the report.

I confirmed it. If I remove the * at beginning of line, the rule works fine: Online demo.

It's very common putting * for each line, so I think the rule should support it.

[platinumazure]

I think we should consider augmenting ast-utils (or creating a new util) which will get the "effective comment text" for a comment and take into account some common patterns, such as JSDoc or just leading * on each line. That way we could get all of our rules that analyze comment text consistent. @mysticatea what do you think?

[sstern6]

Is this still something that needs to be worked on, @platinumazure @mysticatea ?

[platinumazure]

Hi @sstern6, I think so. Would you like to take a crack at this?

[sstern6]

Sure! Though, where does this stand according to your comment with @mysticatea ? Re:

I think we should consider augmenting ast-utils (or creating a new util) which will get the "effective comment text" for a comment and take into account some common patterns, such as JSDoc or just leading * on each line

Should we still go with this approach? @platinumazure

Thanks

[platinumazure]

Go with any approach you like that fixes the issue. 😄

[sstern6]

Lol sounds good, should the user be able to put an unlimited about of * on any given line? Or just one per line? There could be a lot of edge cases

@platinumazure

[sstern6]

Have a few notes/questions:

Context appreciated.....

Looks like we can use https://github.com/eslint/eslint/blob/master/lib/rules/no-warning-comments.js#L114 and inspect the node for the first and last chars of the comment.

Then if the last and first element of the string are eql to *, then remove them and pass the new substring to https://github.com/eslint/eslint/blob/master/lib/rules/no-warning-comments.js#L119.

The regex on line https://github.com/eslint/eslint/blob/master/lib/rules/no-warning-comments.js#L80 needs to also be updated for any content thats location is start and has an*. So, https://github.com/eslint/eslint/blob/master/lib/rules/no-warning-comments.js#L80 should be changed to prefix = "^\\s*.*";.

Though this solution fixes the example above, it doesnt account for if a user were too:

/***
 * todo
 ***/

How many * should we allow the user to have?

Also the implementation described above fixes this example(though you should use // for inline quotes):

/** todo **/

@platinumazure @mysticatea what are your thoughts on this approach?

I could also see how we could update the ast-utils it would just have a higher touch rate and we would need to modify more widely used code. I could see doing that if we were to rethink how we check for JDOC comments and establish some conditions around that.

Thank you

[platinumazure]

@sstern6 That approach sounds promising-- I'm okay with doing a first step now and trying to improve on it later.

What I was originally thinking long-term was, it would be nice if we had a way of getting comment text starting from the first word character (or something like that-- and would be nice to handle Unicode properly), and then no-warning-comments, capitalized-comments, etc. could rely on that to find comment text and ignore extra punctuation.

Instead of trying to add something to ast-utils, though, let's see how far we can get making that sort of change in no-warning-comments first. Then we could easily extract to ast-utils and modify other rules later. Let me know what you think.

[sstern6]

Sounds great @platinumazure. Ill submit a PR shortly, should we make a ticket for the new approach?