no-new-object: warns on non-global Object instantiation

[ljharb]

Tell us about your environment

https://eslint.org/demo#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLW5ldy1vYmplY3Q6IDIsIG5vLXJlZGVjbGFyZTogMCAqL1xudmFyIE9iamVjdCA9IGZ1bmN0aW9uIE9iamVjdCgpIHt9O1xubmV3IE9iamVjdCgpOyIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6NSwic291cmNlVHlwZSI6InNjcmlwdCIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6eyJjb25zdHJ1Y3Rvci1zdXBlciI6MiwiZm9yLWRpcmVjdGlvbiI6MiwiZ2V0dGVyLXJldHVybiI6Miwibm8tYXN5bmMtcHJvbWlzZS1leGVjdXRvciI6Miwibm8tY2FzZS1kZWNsYXJhdGlvbnMiOjIsIm5vLWNsYXNzLWFzc2lnbiI6Miwibm8tY29tcGFyZS1uZWctemVybyI6Miwibm8tY29uZC1hc3NpZ24iOjIsIm5vLWNvbnN0LWFzc2lnbiI6Miwibm8tY29uc3RhbnQtY29uZGl0aW9uIjoyLCJuby1jb250cm9sLXJlZ2V4IjoyLCJuby1kZWJ1Z2dlciI6Miwibm8tZGVsZXRlLXZhciI6Miwibm8tZHVwZS1hcmdzIjoyLCJuby1kdXBlLWNsYXNzLW1lbWJlcnMiOjIsIm5vLWR1cGUta2V5cyI6Miwibm8tZHVwbGljYXRlLWNhc2UiOjIsIm5vLWVtcHR5IjoyLCJuby1lbXB0eS1jaGFyYWN0ZXItY2xhc3MiOjIsIm5vLWVtcHR5LXBhdHRlcm4iOjIsIm5vLWV4LWFzc2lnbiI6Miwibm8tZXh0cmEtYm9vbGVhbi1jYXN0IjoyLCJuby1leHRyYS1zZW1pIjoyLCJuby1mYWxsdGhyb3VnaCI6Miwibm8tZnVuYy1hc3NpZ24iOjIsIm5vLWdsb2JhbC1hc3NpZ24iOjIsIm5vLWlubmVyLWRlY2xhcmF0aW9ucyI6Miwibm8taW52YWxpZC1yZWdleHAiOjIsIm5vLWlycmVndWxhci13aGl0ZXNwYWNlIjoyLCJuby1taXNsZWFkaW5nLWNoYXJhY3Rlci1jbGFzcyI6Miwibm8tbWl4ZWQtc3BhY2VzLWFuZC10YWJzIjoyLCJuby1uZXctc3ltYm9sIjoyLCJuby1vYmotY2FsbHMiOjIsIm5vLW9jdGFsIjoyLCJuby1wcm90b3R5cGUtYnVpbHRpbnMiOjIsIm5vLXJlZGVjbGFyZSI6Miwibm8tcmVnZXgtc3BhY2VzIjoyLCJuby1zZWxmLWFzc2lnbiI6Miwibm8tc2hhZG93LXJlc3RyaWN0ZWQtbmFtZXMiOjIsIm5vLXNwYXJzZS1hcnJheXMiOjIsIm5vLXRoaXMtYmVmb3JlLXN1cGVyIjoyLCJuby11bmRlZiI6Miwibm8tdW5leHBlY3RlZC1tdWx0aWxpbmUiOjIsIm5vLXVucmVhY2hhYmxlIjoyLCJuby11bnNhZmUtZmluYWxseSI6Miwibm8tdW5zYWZlLW5lZ2F0aW9uIjoyLCJuby11bnVzZWQtbGFiZWxzIjoyLCJuby11bnVzZWQtdmFycyI6Miwibm8tdXNlbGVzcy1jYXRjaCI6Miwibm8tdXNlbGVzcy1lc2NhcGUiOjIsIm5vLXdpdGgiOjIsInJlcXVpcmUteWllbGQiOjIsInVzZS1pc25hbiI6MiwidmFsaWQtdHlwZW9mIjoyfSwiZW52Ijp7fX19

What did you expect to happen?
No warning.

What actually happened? Please include the actual, raw output from ESLint.
The object literal notation {} is preferrable. eslint(no-new-object)

Are you willing to submit a pull request to fix this bug?
Sure.

[mdjermanovic]

There is an inconsistency between rules that target particular use of some globals:

• Some rules check just identifier names (e.g., no-new-object).
• Some rules additionally check if the variable is shadowed (e.g., radix).
• Some rules use eslint-utils.ReferenceTracker (e.g., no-obj-calls).

/* eslint no-new-object: "error" */
/* eslint radix: "error" */
/* eslint no-obj-calls: "error" */

/* eslint-env browser*/

new Object(); // error
parseInt(a); // error
JSON(); // error

new window.Object(); // ok
window.parseInt(a); // ok
window.JSON(); // error

function foo(Object, parseInt, JSON) {
    new Object(); // error
    parseInt(a); // ok
    JSON(); // ok
}

Online Demo Link

Bug fix for no-new-object could be to do something like radix, and that shouldn't produce any new warnings.

On the other hand, it might be nice to align the rules of this kind and use ReferenceTracker in all of them.

But, it would also start reporting code such as new window.Object() or var x = Object; new x();, which is an enhancement I guess (maybe even breaking?).

Thoughts on how to proceed?

[ljharb]

Since v7 is coming, I'd definitely want to get all those ReferenceTracker changes in.

In general, I feel like anyone who enables this rule wants those things reported, so i'd consider it a patch.

[mdjermanovic]

I have concerns about cases such as the following, which would be reported if this rule uses ReferenceTracker:

var x = something ? MyClass : Object;

var y = new x();

In this case, user can't just replace something with {}. The code has to be refactored to avoid warning. There could be a more complex example that makes sense but wouldn't satisfy the new version of this rule.

I'm wondering is ReferenceTracker suitable for some rules that are already using it, because it tracks all potential references to the targeted global.

It seems useful in no-obj-calls as it can warn about possible calls to Math, JSON and other non-callable globals in some code paths, which is indeed a runtime error.

On the other hand, this looks like a false positive in the prefer-object-spread rule and an autofix that changes the behavior:

/* eslint prefer-object-spread: ["error"] */

var doSomething = foo ? myFunc : Object.assign;

var x = doSomething({}, a, b); // gets fixed to: var x = { ...a, ...b};

Online Demo

We should probably revisit those rules.

[ljharb]

In that case, that code should be rewritten, as new should never be used with Object :-)

[ljharb]

I certainly do agree, though, that if a reference does not always result in being a thing, it shouldn't be warned/autofixed as being that thing.

[kaicataldo]

@ljharb Agreed that this case is a bug.

@mdjermanovic Thanks for digging into this! Could we create a new meta issue with all the differences in how we handle globals at the moment?

[mdjermanovic]

I'll open an issue for globals, it would be nice to unify that.

As for this issue with no-new-objects, it looks safe for now to just fix the shadowing bug, without ReferenceTracker?