Bug: no-warning-comments does not work with multiline comments

[Reeywhaar]

There is exact same issue, that marked as resolved, but it seems there is regression?

Previous issue: #9884

I will copy description from linked issue, except I've updated the environment.

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

Environment

Node version: 18.2.0
npm version: 8.9.0
Local ESLint version: 8.13.0
Global ESLint version: 8.13.0
Operating System: macOS 12.4

What parser are you using?

@babel/eslint-parser

What did you do?

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": {}
}

/**
 * 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?

Only

// todo

is flagged for no-warning-comments.

Participation

• I am willing to submit a pull request for this issue.

[mdjermanovic]

Hi @Reeywhaar, thanks for the issue!

We were just discussing this in #16090, and it doesn't seem that the original issue #9884 was actually resolved at the time.

/*
 * todo
 */

and

/**
 * todo
 */

should be flagged for no-warning-comments.

I agree that this rule should treat these two comments as starting with 'todo' despite the extra * characters. But since the original issue was closed a long time ago, I would like more opinions from the team, especially because this change can produce more warnings.

[lachlanhunt]

I actually thought about this issue as I was working on #16090. I thought the original intent of location "start" was only to match the start of a single line comment, so I kept it that way in my changes. Setting location to "anywhere" does work more usefully for block comments.

Fixing this, however, would require adding the multiline (m) flag to the regex and stripping out or ignoring asterisks at the start of lines in block comments.

If backwards compatibility is a concern, a possible way to do this would be to introduce a new location value that handled this use case better for block comments, particularly if people consider that location "anywhere" doesn't meet their needs well enough.

[Reeywhaar]

I am not expert in ast, but is there a way to get "effective comment text" as discussed previously?

[lachlanhunt]

I had a go at stripping decorative punctuation in #16120 based on that previous discussion. The difficulty with this, though, is that there needs to be a clearly agreed upon definition of what counts as decorative punctuation in comments that should be stripped.

Rather than just stripping all leading and trailing punctuation, I looked for sequences of a few specific characters, and I picked characters that seemed like the most common to be encountered. Specifically, -, *, =, /, and ~.

It's possible there are edge cases that I missed, and it's also possible there are other edge cases where it shouldn't strip punctuation, but it does.

The documentation for no-warning-comments should also get updated to explain the new functionality before merging it.

[mdjermanovic]

Fixing this, however, would require adding the multiline (m) flag to the regex

I think this issue is only about comments that have "decoration" characters before the effective text starts. If we add the m flag, that would significantly change the meaning of "location": "start". For example, the following comment would be detected as a comment that starts with "todo" term:

/*
    this is not a
    todo comment
*/

[mdjermanovic]

A problem with removing some characters from the comment and then searching for terms in the stripped text is that terms in existing configurations can contain those characters.

For example, if we remove all leading *, then this would no longer match:

/* eslint no-warning-comments: ["error", { terms: ["*TODO*"] } ] */

/*
    *TODO* add something
*/

A backward-compatible solution could be to keep the current behavior as is, and add an option to specify characters that should be skipped at the start of comments.

[lachlanhunt]

That was indeed my interpretation of the issue: Matching the start of any line in a comment, rather than just the start of the first line.

But implementing it the way you suggest would just require stripping decorative punctuation characters and leading whitespace up to the first non-punctuation character. It could be simplified further by only stripping asterisks and whitespace, if that's the desired behaviour. There could still be edge cases, like for example:

{ terms: ["*TODO*"] }

/*
 *TODO* something
 */

Maybe instead of hardcoding the characters considered decorative, that could be configurable, with a sensible default setting.

"no-warning-comments": [1, {
    terms: ["TODO"],
    location: "start",
    commentDecorationChars: "*"
}]

[nzakas]

I believe the current behavior is intentional. I wouldn’t mind adding an option to support the JSDoc-style behavior, maybe allowing a user to specify the text to be skipped at the start of the comment?

I don’t think there’s a good heuristic for knowing which characters after the first asterisk should and shouldn’t be considered by this rule automatically.

[Reeywhaar]

specify the text to be skipped at the start of the comment

As a workaround I've tried to amend rule as follows:

'no-warning-comments': ['warn', { terms: ['*\n  * TODO'], location: 'start' }]

And failed :-)

Also, I think I should mark that comment can be indented:

/*
 * todo:
 */

  /*
   * todo:
   */

So text to be skipped should be kind of regex.

[nzakas]

@eslint/eslint-tsc what do we think about adding a new option for the desired behavior?

[mdjermanovic]

I agree that this should be fixed by adding a new option.

The option could accept an array of characters to (optionally) skip at the start of the comment when checking whether it starts with disallowed terms? Similar to #16103 (comment):

"no-warning-comments": [1, {
    terms: ["TODO"],
    location: "start",
    commentDecorationChars: ["*"]
}]

These characters would be added to the prefix part of the pattern (in the above example, prefix would be "^[\\s*]*") so that they can also appear at the start of configured terms.

[nzakas]

I like it. 👍

[btmills]

@mdjermanovic's suggestion looks good!

[lachlanhunt]

I updated my PR to address the comments here. But I went with a single decoration property that takes a string of characters, rather than an array. It was easier to then just include that within the regex for location "start" using a character class.

given

{ decoration: "*" }

The regex becomes /^[\s\*]*ESCAPED_TERM\b/iu

[mdjermanovic]

String as a list of characters might be confusing. For example, "decoration": "/*" (docs example from #16120) looks like it should skip /* sequences rather than individual characters / and *. I think arrays of characters like ["/", "*"] in configs would be clearer. We can internally .join("") them and work with a string.

[lachlanhunt]

I thought about, but there's no easy way to enforce that each string in the array is a single character anyway. I could change it to allow either a string or array or strings, and then explain in the docs that they're equivalent.

[mdjermanovic]

We can use pattern to enforce single-character string values:

commentDecorationChars: {
    type: "array",
    items: {
        type: "string",
        pattern: "^\\S$"
    },
    minItems: 1,
    uniqueItems: true
}

\S is to disallow specifying whitespace characters to emphasize that they're always included automatically.