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

no-obj-calls should report Atomics() #12234

Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Similar to Math, JSON and Reflect, which the no-obj-calls rule already reports, Atomics is not a function.

Atomics in ECMAScript 2017 spec:

The Atomics object does not have a [[Call]] internal method; it is not possible to invoke the Atomics object as a function.

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

It would be also good to ensure that the reported variables are global.

Please provide some example code that this change will affect:

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

Atomics();

What does the rule currently do for this code?

No errors

What will the rule do after it's changed?

Error: 'Atomics' is not a function.

Are you willing to submit a pull request to implement this change?

Yes. The idea is to add Atomics and also use ReferenceTracker to report only global variables (for all 4 targeted globals) in the same PR.

Would it be okay for a semver-minor? I think that when Reflect was added to this rule it was seen as a bug fix (#7710 #7700).

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 6, 2019
@platinumazure
Copy link
Member

I'm okay with a semver-minor change for the refactor. As for adding Atomics, could we maybe only report it if it's already a known/accepted global? That way, pre-es2017 environments aren't impacted.

@mdjermanovic
Copy link
Member Author

As for adding Atomics, could we maybe only report it if it's already a known/accepted global?

That's the initial idea, to report only if it's a reference to a global variable. Not if it is shadowed, and not if the variable doesn't exist at all. That would apply to Math, JSON, Reflect as well, so the change could also generate fewer warnings.

Global Atomics could be from the enabled es2017 environment, or manually added in config file/global comment (which is probably often the case, because the environment didn't exist until the recent PR #11983).

Would it be okay, or did you mean something else by known/accepted global?

@platinumazure
Copy link
Member

I was referring to having the global be identified as such through env or global config or /* global */ comment, as opposed to an unexpected global reference that would be reported by no-undef or similar. But that might be a pain to implement, so I don't feel strongly about this.

@mdjermanovic
Copy link
Member Author

I think it wouldn't be a problem to implement that check using the ReferenceTracker utility, there is an example in prefer-named-capture-group, I believe it's reporting only global RegExp calls.

@mdjermanovic mdjermanovic self-assigned this Sep 9, 2019
@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 13, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 13, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.