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

Update: Fix call, new and member expressions in no-extra-parens #12302

Merged
merged 1 commit into from Sep 25, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 23, 2019

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

[X] Bug fix

This PR fixes 3 things in no-extra-parens:

  1. False negatives when the object node in a MemberExpression is a NewExpression with parens. Example: (new foo()).bar. This change can generate more warnings by default.
  2. False positives and invalid autofix when chained NewExpression is the calee of a CallExpression or a NewExpression. Example: (new new foo())()
  3. The rule will now report leftParenToken.loc instead of just leftParenToken.loc.start.

Tell us about your environment

  • ESLint Version: 6.4.0
  • Node Version: 10.16.0
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration
module.exports = {
  parserOptions: {
    ecmaVersion: 2015,
  }
};

What did you do? Please include the actual source code causing the issue.

Demo link

/* eslint no-extra-parens:error */

(new foo()).bar;

(new new foo())();

new (new new foo())(bar);

What did you expect to happen?

1 error, for the first line only.

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

2 errors and invalid autofix for the other two lines.

/* eslint no-extra-parens:error */

// false negative
(new foo()).bar; 

// false positive and invalid autofix - effectively removes call expression
new new foo()(); 

// false positive and invalid autofix - `bar` becomes argument of the second `new`
new new new foo()(bar); 

What changes did you make? (Give an overview)

  1. Additional check in MemberExpression.
  2. Fixed bug in isNewExpressionWithParens function.
  3. The rule now reports the full left paren's loc instead of loc.start.

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

Please verify that these are indeed false positives/negatives, and that the false negative wasn't intentional (I guess it's a bug as there were no test cases to confirm that it was intentional).

@eslint eslint bot added the triage label Sep 23, 2019
@g-plane g-plane added bug evaluating rule and removed triage labels Sep 24, 2019
@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 25, 2019

I'm a bit confused. Parents are needed for the first expression: (new foo()).bar is not the same thing as new foo().bar. And is new new syntax even valid? If it is, it's most likely a mistake, I think.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 25, 2019

Parents are needed for the first expression: (new foo()).bar is not the same thing as new foo().bar.

The (new foo()).bar and new foo().bar are the same thing. I guess you are confused with (new foo).bar.

And is new new syntax even valid?

Yes, valid. Constructors can return another constructor.

function Foo() {
    return Bar
}

function Bar() {
}

var bar = new new Foo // a Bar's instance.

If it is, it's most likely a mistake, I think.

I agree. But the problem is no-extra-parens changes semantics unintentionally. It's a bug in the rule.

@mysticatea mysticatea added accepted and removed evaluating labels Sep 25, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 25, 2019

I'm a bit confused. Parents are needed for the first expression: (new foo()).bar is not the same thing as new foo().bar.

It's the same thing, but the extra parens could be indeed helpful in this situation to avoid confusion. That's why I was wondering was it intentionally omitted. However, there is no test case to show this intention, so I believe it's a bug (unintentional oversight).

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 25, 2019

Ah, I did mix it up with (new foo).bar. In that case, I guess those fixes are fine. They do cover an extreme edge-cases, but it doesn't hurt.

Copy link
Member

platinumazure left a comment

LGTM, thanks!

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@kaicataldo kaicataldo merged commit 11ae6fc into master Sep 25, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
continuous-integration Build #20190923.2 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo kaicataldo deleted the noextraparens-new branch Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.