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

[mdjermanovic]

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).

[ilyavolodin]

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]

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.

[mdjermanovic]

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]

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.

[platinumazure]

LGTM, thanks!

[kaicataldo]

LGTM, thanks!