Update: handle uncommon linebreaks consistently in rules (fixes #7949) #8049

not-an-aardvark · 2017-02-08T23:18:41Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix (see #7949)

What changes did you make? (Give an overview)

Currently, rules that deal with line-splitting need to include a list of JS linebreaks in some form (e.g. in a regex character class). As a result, splitting a string into JS lines is implemented in 22 different places in the codebase. As described in #7949, not all of these implementations are correct (many of them forget to account for \r, \u2028, and \u2029).

This commit updates ast-utils.js with the following properties:

astUtils.LINEBREAKS: a Set containing all JS linebreaks
astUtils.LINEBREAK_MATCHER: a regular expression to match JS linebreaks
astUtils.createGlobalLinebreakMatcher: a function that returns a new global regex to match JS linebreaks

Additionally, it updates the other line-splitting code to use these constants.

Observable changes:

The error message for newline-per-chained-call will handle property names with \u2028 and \u2029 the same way it handles names with \n.
no-multi-str will now report an error for strings that contain \r, \u2028, or \u2029. (Previously, it only reported strings containing \n.)
no-unused-vars will now report the correct location if a /* global foo */ comment contains a linebreak other than \n. (Previously, it would report an invalid location.)
spaced-comment will no longer report an error if a comment contains tokens in the exceptions option followed by \u2028 or \u2029.

Is there anything you'd like reviewers to focus on?

Nothing in particular

mention-bot · 2017-02-08T23:18:43Z

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @vitorbal and @nzakas to be potential reviewers.

ilyavolodin · 2017-02-09T01:49:19Z

lib/rules/no-multi-spaces.js

@@ -93,7 +93,7 @@ module.exports = {
                const sourceCode = context.getSourceCode(),
                    source = sourceCode.getText(),
                    allComments = sourceCode.getAllComments(),
-                    pattern = /[^\n\r\u2028\u2029\t ].? {2,}/g;  // note: repeating space
+                    pattern = /[^\s].? {2,}/g;  // note: repeating space


The new regEx is a bit more restrictive then before. It will include tabs and spaces on top of the linebreaks in the "not" group. Is this intentional?

I might be misunderstanding -- the old "not" group also had tabs and spaces.

Oh, you are right, sorry. I didn't notice white space and \t towards the end.

Actually, looking at the reference, \s is the same as [\f\n\r\t\v\u00A0\u2028\u2029], so it looks like you still picked up a few extra constrains. Not sure if it matters.

Hmm, looks like you're right. I'll switch it back.

The effect of this is that the following code would not be reported:

foo\f\f + 1

ilyavolodin · 2017-02-09T01:51:19Z

lib/rules/no-trailing-spaces.js

@@ -34,7 +40,7 @@ module.exports = {
    create(context) {
        const sourceCode = context.getSourceCode();

-        const BLANK_CLASS = "[ \t\u00a0\u2000-\u200b\u2028\u2029\u3000]",
+        const BLANK_CLASS = "[ \t\u00a0\u2000-\u200b\u3000]",


This looks correct, but I'm not sure why it originally included line breaks? Maybe there was a reason for it?

I think the linebreaks are redundant here because the regex is only executed on elements of sourceCode.lines.

ilyavolodin · 2017-02-09T01:52:09Z

lib/rules/no-unused-vars.js

-                line: baseLoc.line + lineInComment,
-                column
-            };
+            return astUtils.getLocationFromRangeIndex(sourceCode, comment.range[0] + 2 + getColumnInComment(variable, comment));


This looks unrelated to the topic of this PR, but I think we can let it slide:-)

Fair enough. (This actually is a place where no-unused-vars had a bug due to a bad linebreak regex, but I suppose it could have been fixed in a different way.)

Currently, rules that deal with line-splitting need to include a list of JS linebreaks in some form (e.g. in a regex character class). As a result, splitting a string into JS lines is implemented in 22 different places in the codebase. As described in #7949, not all of these implementations are correct (many of them forget to account for \r, \u2028, and \u2029. This commit updates ast-utils.js with the following properties: * `astUtils.LINEBREAKS`: a Set containing all JS linebreaks * `astUtils.LINEBREAK_MATCHER`: a regular expression to match JS linebreaks * `astUtils.createGlobalLinebreakMatcher`: a function that returns a new global regex to match JS linebreaks Additionally, it updates the other line-splitting code to use these constants. Observable changes: * The error message for `newline-per-chained-call` will handle property names with \u2028 and \u2029 the same way it handles names with \n. * `no-multi-str` will now report an error for strings that contain \r, \u2028, or \u2029. (Previously, it only reported strings containing \n.) * `no-unused-vars` will now report the correct location if a `/* global foo */` comment contains a linebreak other than \n. (Previously, it would report an invalid location.) * `spaced-comment` will no longer report an error if a comment contains tokens in the `exceptions` option followed by \u2028 or \u2029.

eslintbot · 2017-02-12T02:02:26Z

LGTM

eslintbot · 2017-02-13T02:46:39Z

LGTM

ilyavolodin · 2017-02-13T03:00:18Z

LGTM. Thanks!

ilyavolodin approved these changes Feb 9, 2017

View reviewed changes

not-an-aardvark force-pushed the unify-linebreak-logic branch from 4a1f724 to 79ea45e Compare February 9, 2017 05:28

not-an-aardvark added 2 commits February 11, 2017 21:02

Fix build on Node 4

851b533

not-an-aardvark force-pushed the unify-linebreak-logic branch from 79ea45e to 851b533 Compare February 12, 2017 02:02

Avoid changing whitespace logic in no-multi-spaces

0acabf1

ilyavolodin merged commit acc3301 into master Feb 13, 2017

not-an-aardvark deleted the unify-linebreak-logic branch February 13, 2017 03:02

This was referenced Feb 20, 2017

Update dependency eslint to version 3.19.0 singapore/lint-condo#244

Merged

Update dependency eslint to version 3.16.0 renovatebot/renovate#111

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: handle uncommon linebreaks consistently in rules (fixes #7949) #8049

Update: handle uncommon linebreaks consistently in rules (fixes #7949) #8049

not-an-aardvark commented Feb 8, 2017

mention-bot commented Feb 8, 2017

ilyavolodin Feb 9, 2017

not-an-aardvark Feb 9, 2017

ilyavolodin Feb 9, 2017

ilyavolodin Feb 9, 2017 •

edited

not-an-aardvark Feb 13, 2017

ilyavolodin Feb 9, 2017

not-an-aardvark Feb 9, 2017

ilyavolodin Feb 9, 2017

not-an-aardvark Feb 9, 2017

eslintbot commented Feb 12, 2017

eslintbot commented Feb 13, 2017

ilyavolodin commented Feb 13, 2017

Update: handle uncommon linebreaks consistently in rules (fixes #7949) #8049

Update: handle uncommon linebreaks consistently in rules (fixes #7949) #8049

Conversation

not-an-aardvark commented Feb 8, 2017

mention-bot commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyavolodin Feb 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Feb 12, 2017

eslintbot commented Feb 13, 2017

ilyavolodin commented Feb 13, 2017

ilyavolodin Feb 9, 2017 •

edited