Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: add unescaped regexp dot rule to linter #11834

Closed

Conversation

@mscdex
Copy link
Contributor

commented Mar 13, 2017

This adds a custom rule to the linter that checks for unescaped "literal" dots in regular expressions. For regexp literals this is straightforward, but it's trickier for RegExp() usage because of possible variable text passed to the constructor. This rule tries its best in the constructor scenario by concatenating any/all of the string literals passed to it and checking that.

CI: https://ci.nodejs.org/job/node-test-pull-request/6825/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • tools
  • test

@mscdex mscdex added test tools labels Mar 13, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2017

tools/eslint-rules/unescaped-regexp-dot.js Outdated
const utilsPath = path.join(__dirname, '..', 'eslint', 'lib', 'ast-utils.js');
const astUtils = require(utilsPath);
const getLocationFromRangeIndex = astUtils.getLocationFromRangeIndex;
const getRangeIndexFromLocation = astUtils.getRangeIndexFromLocation;

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Mar 14, 2017

Member

These methods aren't part of the public API, and were removed in ESLint v3.17.0. (Using them for now is probably fine, but they'll need to be changed to sourceCode.getLocFromIndex and sourceCode.getIndexFromLoc when ESLint is upgraded.)

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 14, 2017

Author Contributor

Yes, I know.

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 14, 2017

Author Contributor

FWIW I've added a check to support the later versions too.

tools/eslint-rules/unescaped-regexp-dot.js Outdated
break;
case '.':
if (!escaping) {
if (!inCharClass && (i + 1) < str.length) {

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Mar 14, 2017

Member

Would we want to report the node when i + 1 === str.length? This would cover the case where the . is at the end of the regex, e.g.

assert.throws(func, /this error message ends with a dot./);

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 14, 2017

Author Contributor

Changed.

tools/eslint-rules/unescaped-regexp-dot.js Outdated
return {
Literal: function(node) {
if (inRegExp && typeof node.value === 'string' && node.value.length)
regexpBuffer.push([node, node.value]);

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Mar 14, 2017

Member

This could cause some weird behavior if the argument to RegExp isn't just string concatenation:

// 'foo.bar' gets interpreted as a regex pattern
new RegExp(function() {
  return crypto.createHash('sha1').update('foo.bar').digest('hex');
}());

Or it could handle character classes incorrectly:

const closingBracket = ']';

new RegExp('[abc' + closingBracket + '.');

There's no perfect solution to this, so it's probably fine as-is if we're not running into cases like this.

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 14, 2017

Author Contributor

Regarding the first example, I originally had it coded differently and actually checked parent nodes when encountering string literals, but that seemed more complicated.

Regarding the second example, I pointed that out in the PR description. I doubt that would ever become a problem in practice though.

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 14, 2017

Author Contributor

I've re-added the strict parent node checking.

@mscdex mscdex force-pushed the mscdex:tools-eslint-regex-dot-escape-rule branch 2 times, most recently Mar 14, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

I've addressed raised concerns and added support for template literals. These new changes resulted in several more instances being found.

CI: https://ci.nodejs.org/job/node-test-pull-request/6832/

@fhinkel

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

I think this needs a rebase.

@fhinkel

This comment has been minimized.

Copy link
Member

commented Mar 26, 2017

ping @mscdex

@mscdex mscdex force-pushed the mscdex:tools-eslint-regex-dot-escape-rule branch to 9153949 Mar 29, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

@fhinkel Rebased.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

@fhinkel

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

Thanks. Landed in 61ebfa8

@fhinkel fhinkel closed this Mar 29, 2017

fhinkel added a commit that referenced this pull request Mar 29, 2017
tools: add unescaped regexp dot rule to linter
PR-URL: #11834
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@mscdex mscdex deleted the mscdex:tools-eslint-regex-dot-escape-rule branch Mar 29, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 17, 2017

Are you willing to backport this to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@Trott Trott referenced this pull request Aug 17, 2017
2 of 2 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

ping one more time re: backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.