Update: Report constructor calls in no-obj-calls

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[X] Changes an existing rule

Note: this PR also removes Math and JSON from the no-new-wrappers rule.

What rule do you want to change?

no-obj-calls

Does this change cause the rule to produce more or fewer warnings?

more

How will the change be implemented? (New option, new default behavior, etc.)?

new default behavior

Please provide some example code that this change will affect:

/*eslint no-obj-calls: "error"*/
/*eslint-env es2017*/

var newMath = new Math();

var newJSON = new JSON();

var newReflect = new Reflect();

var newAtomics = new Atomics();

What does the rule currently do for this code?

no errors

What will the rule do after it's changed?

4 errors

What changes did you make? (Give an overview)

Updated no-obj-calls to warn on NewExpression as well.

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

As suggested in #11984 (comment), no-obj-calls instead of no-new-wrappers should report new Math() and new JSON(). I guess it makes sense to report all 4 objects when used with new, it's always a runtime error.

I think there is no need for a new messageId.

[ljharb]

Would it also be worth throwing on new $BuiltIn.prototype for every builtin that's not a function?

[mdjermanovic]

Would it also be worth throwing on new $BuiltIn.prototype for every builtin that's not a function?

Like new Math.prototype? Wouldn't that always throw for all functions as well, e.g., new RegExp.prototype

[ljharb]

RegExp.prototype isn't a function; Function.prototype is :-) so i mean like, new Boolean.prototype(), new Object.prototype(), etc.

[mdjermanovic]

RegExp.prototype isn't a function; Function.prototype is :-) so i mean like, new Boolean.prototype(), new Object.prototype(), etc.

I think this isn't related to globals, new foo.prototype() is always an error (assuming the common meaning of the prototype property).

[ljharb]

Yes, so is new Math().

I’m suggesting that the “no object calls” rule should warn on builtin objects that aren’t functions, which includes both Math and Boolean.prototype.

[mdjermanovic]

Yes, so is new Math().

I’m suggesting that the “no object calls” rule should warn on builtin objects that aren’t functions, which includes both Math and Boolean.prototype.

I understand the logic now - for every builtin function BuiltIn, BuiltIn.prototype is also a builtin object :)

Makes sense (just not sure how often people write new anything.prototype()), though I think that should be a separate proposal?

[ljharb]

Perhaps, just seems like an easy addition and in the same spirit of the RFC.

[kaicataldo]

I support this change, though I think we should mark it as breaking since the rule is catching a whole new class of error.

[ljharb]

@kaicataldo it seems like the same class to me - using new on something that can’t possibly do anything but throw.

[nzakas]

I’m not a fan of checking for prototypes as I just don’t think that’s a common enough mistake. However, if others disagree please open a separate issue to discuss. We shouldn’t add anything more to this PR.

[ljharb]

I mean, fair, but is new JSON or new Math common?

[nzakas]

@mdjermanovic it looks like there’s a merge conflict, can you rebase and fix the conflicts?

[mdjermanovic]

Rebased, fixed conflicts, and added some tests that combine changes from this PR with changes from #12774.

[mdjermanovic]

If it would be easier, I can add changes from #11984 to this PR.

This PR and #11984 should be, ideally, merged at the same time anyway.

[nzakas]

That seems like a good idea to get #11984 unblocked. 👍 to adding those commits to this PR

[mdjermanovic]

Done!

(tests are failing only because of #13076).

[nzakas]

Merged #13076. Can you try rebasing to see if that makes the CI happy?

[mdjermanovic]

It works well now 🎉

[nzakas]

Awesome, thanks!