From b3de940e6857974c18c7b836a3ab710cf4b291d4 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Sun, 3 Jul 2022 20:56:17 +1000 Subject: [PATCH 1/7] fix: no-warning-comments rule escapes special RegEx characters in terms --- lib/rules/no-warning-comments.js | 71 +++++-------- tests/lib/rules/no-warning-comments.js | 134 ++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 49 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 40c89b19eb6..9ada0c28acf 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -64,59 +64,36 @@ module.exports = { */ function convertToRegExp(term) { const escaped = escapeRegExp(term); - const wordBoundary = "\\b"; - const eitherOrWordBoundary = `|${wordBoundary}`; - let prefix; /* - * If the term ends in a word character (a-z0-9_), ensure a word - * boundary at the end, so that substrings do not get falsely - * matched. eg "todo" in a string such as "mastodon". - * If the term ends in a non-word character, then \b won't match on - * the boundary to the next non-word character, which would likely - * be a space. For example `/\bFIX!\b/.test('FIX! blah') === false`. - * In these cases, use no bounding match. Same applies for the - * prefix, handled below. + * When matching at the start, ignore leading whitespace, and + * there's no need to worry about word boundaries. + * + * These expressions for the prefix and suffix are designed as follows: + * ^ handles any terms at the beginning of a line + * e.g. terms ["TODO"] matches `//TODO something` + * $ handles any terms at the end of a line + * e.g. terms ["TODO"] matches `// something TODO` + * \s* handles optional leading spaces (for "start" location only) + * e.g. terms ["TODO"] matches `// TODO something` + * \W handles terms preceded/followed by non-word characters, especially where there's no word boundary. + * e.g. terms: ["!TODO", "TODO!"] matches `// !!!TODO blah` or `// TODO!!! blah` + * \b handles terms preceded/followed by word boundary + * e.g. terms: ["!FIX", "FIX!"] matches `// FIX!something` or `// something!FIX` + * terms: ["FIX"] matches `// FIX!` or `// !FIX`, but not `// fixed or affix` */ - const suffix = /\w$/u.test(term) ? "\\b" : ""; - - if (location === "start") { - - /* - * When matching at the start, ignore leading whitespace, and - * there's no need to worry about word boundaries. - */ - prefix = "^\\s*"; - } else if (/^\w/u.test(term)) { - prefix = wordBoundary; - } else { - prefix = ""; - } - - if (location === "start") { - - /* - * For location "start" the regex should be - * ^\s*TERM\b. This checks the word boundary - * at the beginning of the comment. - */ - return new RegExp(prefix + escaped + suffix, "iu"); - } + const prefix = location === "start" ? "^\\s*" : "(?:^|\\b|\\W)"; + const suffix = "(?:\\b|\\W|$)"; + const flags = "iu"; // Case-insensitive with Unicode case folding. /* - * For location "anywhere" the regex should be - * \bTERM\b|\bTERM\b, this checks the entire comment - * for the term. + * For location "start" the regex should be + * /^\s*ESCAPED_TERM(?:\b|\W|$)/iu. + * + * For location "anywhere" the regex should be: + * /(?:^|\b|\W)ESCAPED_TERM(?:\b|\W|$)/iu */ - return new RegExp( - prefix + - escaped + - suffix + - eitherOrWordBoundary + - term + - wordBoundary, - "iu" - ); + return new RegExp(`${prefix}${escaped}${suffix}`, flags); } const warningRegExps = warningTerms.map(convertToRegExp); diff --git a/tests/lib/rules/no-warning-comments.js b/tests/lib/rules/no-warning-comments.js index c2689defab1..398e6780d86 100644 --- a/tests/lib/rules/no-warning-comments.js +++ b/tests/lib/rules/no-warning-comments.js @@ -34,10 +34,10 @@ ruleTester.run("no-warning-comments", rule, { "/* any block comment with TODO, FIXME or XXX */", "/* any block comment with (TODO, FIXME's or XXX!) */", { code: "// comments containing terms as substrings like TodoMVC", options: [{ terms: ["todo"], location: "anywhere" }] }, - { code: "// special regex characters don't cause problems", options: [{ terms: ["[aeiou]"], location: "anywhere" }] }, + { code: "// special regex characters don't cause a problem", options: [{ terms: ["[aeiou]"], location: "anywhere" }] }, "/*eslint no-warning-comments: [2, { \"terms\": [\"todo\", \"fixme\", \"any other term\"], \"location\": \"anywhere\" }]*/\n\nvar x = 10;\n", { code: "/*eslint no-warning-comments: [2, { \"terms\": [\"todo\", \"fixme\", \"any other term\"], \"location\": \"anywhere\" }]*/\n\nvar x = 10;\n", options: [{ location: "anywhere" }] }, - { code: "foo", options: [{ terms: ["foo-bar"] }] } + { code: "// foo", options: [{ terms: ["foo-bar"] }] } ], invalid: [ { @@ -257,6 +257,136 @@ ruleTester.run("no-warning-comments", rule, { } } ] + }, + { + code: "// Comment ending with term followed by punctuation TODO!", + options: [{ terms: ["todo"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "todo", + comment: "Comment ending with term followed by..." + } + } + ] + }, + { + code: "// Comment ending with term including punctuation TODO!", + options: [{ terms: ["todo!"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "todo!", + comment: "Comment ending with term including..." + } + } + ] + }, + { + code: "// Comment ending with term including punctuation followed by more TODO!!!", + options: [{ terms: ["todo!"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "todo!", + comment: "Comment ending with term including..." + } + } + ] + }, + { + code: "// !TODO comment starting with term", + options: [{ terms: ["todo"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "todo", + comment: "!TODO comment starting with term" + } + } + ] + }, + { + code: "// !TODO comment starting with term including punctuation", + options: [{ terms: ["!todo"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "!todo", + comment: "!TODO comment starting with term..." + } + } + ] + }, + { + code: "// !!!TODO comment starting with term including punctuation preceded by more", + options: [{ terms: ["!todo"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "!todo", + comment: "!!!TODO comment starting with term..." + } + } + ] + }, + { + code: "// FIX!term ending with punctuation followed word character", + options: [{ terms: ["FIX!"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "FIX!", + comment: "FIX!term ending with punctuation..." + } + } + ] + }, + { + code: "// Term starting with punctuation preceded word character!FIX", + options: [{ terms: ["!FIX"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "!FIX", + comment: "Term starting with punctuation preceded..." + } + } + ] + }, + { + code: "//!XXX comment starting with no spaces (anywhere)", + options: [{ terms: ["!xxx"], location: "anywhere" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "!xxx", + comment: "!XXX comment starting with no spaces..." + } + } + ] + }, + { + code: "//!XXX comment starting with no spaces (start)", + options: [{ terms: ["!xxx"], location: "start" }], + errors: [ + { + messageId: "unexpectedComment", + data: { + matchedTerm: "!xxx", + comment: "!XXX comment starting with no spaces..." + } + } + ] } ] }); From b5e779a23fb8915f9f1721cc0252fc3ec00e62d0 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Wed, 6 Jul 2022 09:49:47 +1000 Subject: [PATCH 2/7] Fix unit test and explanatory comment --- lib/rules/no-warning-comments.js | 2 +- tests/lib/rules/no-warning-comments.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 9ada0c28acf..6c91b7607d4 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -70,7 +70,7 @@ module.exports = { * there's no need to worry about word boundaries. * * These expressions for the prefix and suffix are designed as follows: - * ^ handles any terms at the beginning of a line + * ^ handles any terms at the beginning of a comment. * e.g. terms ["TODO"] matches `//TODO something` * $ handles any terms at the end of a line * e.g. terms ["TODO"] matches `// something TODO` diff --git a/tests/lib/rules/no-warning-comments.js b/tests/lib/rules/no-warning-comments.js index 398e6780d86..6d4137a5e84 100644 --- a/tests/lib/rules/no-warning-comments.js +++ b/tests/lib/rules/no-warning-comments.js @@ -298,14 +298,14 @@ ruleTester.run("no-warning-comments", rule, { ] }, { - code: "// !TODO comment starting with term", + code: "// !TODO comment starting with term preceded by punctuation", options: [{ terms: ["todo"], location: "anywhere" }], errors: [ { messageId: "unexpectedComment", data: { matchedTerm: "todo", - comment: "!TODO comment starting with term" + comment: "!TODO comment starting with term..." } } ] From 2a2569a12287553a0933b1d225e0ab2cc0eebae8 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Fri, 8 Jul 2022 12:04:12 +1000 Subject: [PATCH 3/7] Fix comment about $ matching --- lib/rules/no-warning-comments.js | 39 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 6c91b7607d4..66814e91e83 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -22,7 +22,7 @@ module.exports = { docs: { description: "Disallow specified warning terms in comments", recommended: false, - url: "https://eslint.org/docs/rules/no-warning-comments" + url: "https://eslint.org/docs/rules/no-warning-comments", }, schema: [ @@ -32,20 +32,21 @@ module.exports = { terms: { type: "array", items: { - type: "string" - } + type: "string", + }, }, location: { - enum: ["start", "anywhere"] - } + enum: ["start", "anywhere"], + }, }, - additionalProperties: false - } + additionalProperties: false, + }, ], messages: { - unexpectedComment: "Unexpected '{{matchedTerm}}' comment: '{{comment}}'." - } + unexpectedComment: + "Unexpected '{{matchedTerm}}' comment: '{{comment}}'.", + }, }, create(context) { @@ -72,7 +73,7 @@ module.exports = { * These expressions for the prefix and suffix are designed as follows: * ^ handles any terms at the beginning of a comment. * e.g. terms ["TODO"] matches `//TODO something` - * $ handles any terms at the end of a line + * $ handles any terms at the end of a comment * e.g. terms ["TODO"] matches `// something TODO` * \s* handles optional leading spaces (for "start" location only) * e.g. terms ["TODO"] matches `// TODO something` @@ -132,12 +133,14 @@ module.exports = { const matches = commentContainsWarningTerm(comment); - matches.forEach(matchedTerm => { + matches.forEach((matchedTerm) => { let commentToDisplay = ""; let truncated = false; for (const c of comment.trim().split(/\s+/u)) { - const tmp = commentToDisplay ? `${commentToDisplay} ${c}` : c; + const tmp = commentToDisplay + ? `${commentToDisplay} ${c}` + : c; if (tmp.length <= CHAR_LIMIT) { commentToDisplay = tmp; @@ -152,10 +155,8 @@ module.exports = { messageId: "unexpectedComment", data: { matchedTerm, - comment: `${commentToDisplay}${ - truncated ? "..." : "" - }` - } + comment: `${commentToDisplay}${truncated ? "..." : ""}`, + }, }); }); } @@ -165,9 +166,9 @@ module.exports = { const comments = sourceCode.getAllComments(); comments - .filter(token => token.type !== "Shebang") + .filter((token) => token.type !== "Shebang") .forEach(checkComment); - } + }, }; - } + }, }; From e66b7aaee68607c93afc46e60a9838751518eef2 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Fri, 8 Jul 2022 20:49:56 +1000 Subject: [PATCH 4/7] Undo accidental formatting. --- lib/rules/no-warning-comments.js | 37 ++++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 66814e91e83..82aae28831a 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -22,7 +22,7 @@ module.exports = { docs: { description: "Disallow specified warning terms in comments", recommended: false, - url: "https://eslint.org/docs/rules/no-warning-comments", + url: "https://eslint.org/docs/rules/no-warning-comments" }, schema: [ @@ -32,21 +32,20 @@ module.exports = { terms: { type: "array", items: { - type: "string", - }, + type: "string" + } }, location: { - enum: ["start", "anywhere"], - }, + enum: ["start", "anywhere"] + } }, - additionalProperties: false, - }, + additionalProperties: false + } ], messages: { - unexpectedComment: - "Unexpected '{{matchedTerm}}' comment: '{{comment}}'.", - }, + unexpectedComment: "Unexpected '{{matchedTerm}}' comment: '{{comment}}'." + } }, create(context) { @@ -133,14 +132,12 @@ module.exports = { const matches = commentContainsWarningTerm(comment); - matches.forEach((matchedTerm) => { + matches.forEach(matchedTerm => { let commentToDisplay = ""; let truncated = false; for (const c of comment.trim().split(/\s+/u)) { - const tmp = commentToDisplay - ? `${commentToDisplay} ${c}` - : c; + const tmp = commentToDisplay ? `${commentToDisplay} ${c}` : c; if (tmp.length <= CHAR_LIMIT) { commentToDisplay = tmp; @@ -155,8 +152,10 @@ module.exports = { messageId: "unexpectedComment", data: { matchedTerm, - comment: `${commentToDisplay}${truncated ? "..." : ""}`, - }, + comment: `${commentToDisplay}${ + truncated ? "..." : "" + }` + } }); }); } @@ -166,9 +165,9 @@ module.exports = { const comments = sourceCode.getAllComments(); comments - .filter((token) => token.type !== "Shebang") + .filter(token => token.type !== "Shebang") .forEach(checkComment); - }, + } }; - }, + } }; From 7b2f71e1e2b600589ce657789c92a439cc5e5e05 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Sat, 9 Jul 2022 10:12:33 +1000 Subject: [PATCH 5/7] Only include \W when the term starts or ends with a non-word character --- lib/rules/no-warning-comments.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 82aae28831a..ed74dcf70f1 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -82,8 +82,8 @@ module.exports = { * e.g. terms: ["!FIX", "FIX!"] matches `// FIX!something` or `// something!FIX` * terms: ["FIX"] matches `// FIX!` or `// !FIX`, but not `// fixed or affix` */ - const prefix = location === "start" ? "^\\s*" : "(?:^|\\b|\\W)"; - const suffix = "(?:\\b|\\W|$)"; + const prefix = location === "start" ? "^\\s*" : `(?:^|\\b${/^\W/u.test(escaped) ? "|\\W" : ""})`; + const suffix = `(?:\\b${/\W$/u.test(escaped) ? "|\\W" : ""}|$)`; const flags = "iu"; // Case-insensitive with Unicode case folding. /* From 8d0599856b1507171390aca317ee66d42cae90fd Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Sat, 9 Jul 2022 23:03:30 +1000 Subject: [PATCH 6/7] Performance improvements --- lib/rules/no-warning-comments.js | 33 ++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index ed74dcf70f1..5077f358d33 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -76,22 +76,39 @@ module.exports = { * e.g. terms ["TODO"] matches `// something TODO` * \s* handles optional leading spaces (for "start" location only) * e.g. terms ["TODO"] matches `// TODO something` - * \W handles terms preceded/followed by non-word characters, especially where there's no word boundary. - * e.g. terms: ["!TODO", "TODO!"] matches `// !!!TODO blah` or `// TODO!!! blah` + * !|[^!] handles terms preceded/followed by non-word characters (e.g. punctuation). + * In this case, the term can be followed by anything. + * This is equivalent to ".", but performance testing with eslint shows this is orders of magnitude faster. + * e.g. terms: ["!TODO", "TODO!"] matches `// !!!TODO something` or `// TODO!!! something` + * terms: ["TODO:"] matches `// TODO:something" * \b handles terms preceded/followed by word boundary * e.g. terms: ["!FIX", "FIX!"] matches `// FIX!something` or `// something!FIX` * terms: ["FIX"] matches `// FIX!` or `// !FIX`, but not `// fixed or affix` */ - const prefix = location === "start" ? "^\\s*" : `(?:^|\\b${/^\W/u.test(escaped) ? "|\\W" : ""})`; - const suffix = `(?:\\b${/\W$/u.test(escaped) ? "|\\W" : ""}|$)`; + const wordBoundary = "\\b"; + const anyChar = "!|[^!]"; + let prefix; + + if (location === "start") { + prefix = "^\\s*"; + } else if (/^\w/u.test(term)) { + prefix = wordBoundary; + } else { + prefix = `(?:^|${anyChar})`; + } + + const suffix = /\w$/u.test(term) ? wordBoundary : `(?:${anyChar}|$)`; const flags = "iu"; // Case-insensitive with Unicode case folding. /* - * For location "start" the regex should be - * /^\s*ESCAPED_TERM(?:\b|\W|$)/iu. + * For location "start", the typical regex is: + * /^\s*ESCAPED_TERM\b/iu. + * + * For location "anywhere" the typical regex is + * /\bESCAPED_TERM\b/iu * - * For location "anywhere" the regex should be: - * /(?:^|\b|\W)ESCAPED_TERM(?:\b|\W|$)/iu + * If it starts or ends with non-word character, the prefix and suffix are modified + * to match the start or end of the comment, or any other character. */ return new RegExp(`${prefix}${escaped}${suffix}`, flags); } From 631506041010a8dd55405552b02bae405342c957 Mon Sep 17 00:00:00 2001 From: Lachlan Hunt Date: Sat, 9 Jul 2022 23:14:06 +1000 Subject: [PATCH 7/7] Simplify regexes --- lib/rules/no-warning-comments.js | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/lib/rules/no-warning-comments.js b/lib/rules/no-warning-comments.js index 5077f358d33..4f867cbb707 100644 --- a/lib/rules/no-warning-comments.js +++ b/lib/rules/no-warning-comments.js @@ -76,28 +76,21 @@ module.exports = { * e.g. terms ["TODO"] matches `// something TODO` * \s* handles optional leading spaces (for "start" location only) * e.g. terms ["TODO"] matches `// TODO something` - * !|[^!] handles terms preceded/followed by non-word characters (e.g. punctuation). - * In this case, the term can be followed by anything. - * This is equivalent to ".", but performance testing with eslint shows this is orders of magnitude faster. - * e.g. terms: ["!TODO", "TODO!"] matches `// !!!TODO something` or `// TODO!!! something` - * terms: ["TODO:"] matches `// TODO:something" * \b handles terms preceded/followed by word boundary * e.g. terms: ["!FIX", "FIX!"] matches `// FIX!something` or `// something!FIX` * terms: ["FIX"] matches `// FIX!` or `// !FIX`, but not `// fixed or affix` */ const wordBoundary = "\\b"; - const anyChar = "!|[^!]"; - let prefix; + + let prefix = ""; if (location === "start") { prefix = "^\\s*"; } else if (/^\w/u.test(term)) { prefix = wordBoundary; - } else { - prefix = `(?:^|${anyChar})`; } - const suffix = /\w$/u.test(term) ? wordBoundary : `(?:${anyChar}|$)`; + const suffix = /\w$/u.test(term) ? wordBoundary : ""; const flags = "iu"; // Case-insensitive with Unicode case folding. /* @@ -107,8 +100,7 @@ module.exports = { * For location "anywhere" the typical regex is * /\bESCAPED_TERM\b/iu * - * If it starts or ends with non-word character, the prefix and suffix are modified - * to match the start or end of the comment, or any other character. + * If it starts or ends with non-word character, the prefix and suffix empty, respectively. */ return new RegExp(`${prefix}${escaped}${suffix}`, flags); }