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

require-atomic-updates: document.title (false positive) #11967

Closed
Lonniebiz opened this issue Jul 8, 2019 · 4 comments
Closed

require-atomic-updates: document.title (false positive) #11967

Lonniebiz opened this issue Jul 8, 2019 · 4 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@Lonniebiz
Copy link

Lonniebiz commented Jul 8, 2019

I'm using eslint 6.0.1 and believe I'm getting a false positive for the require-atomic-updates rule. Here's a code sample which reproduces the false-positive:

async function test()
{
	document.createElement("div");
	await Promise.resolve();
	document.title = "Test";
}
test();

The line document.title = "Test"; (above) produces this error:

Possible race condition: `document.title` might be reassigned based 
on an outdated value of `document.title`. (require-atomic-updates)

However, if I change document.title to window.document.title eslint does NOT produce the error:

async function test()
{
	document.createElement("div");
	await Promise.resolve();
	window.document.title = "Test";
}
test();
@Lonniebiz Lonniebiz added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jul 8, 2019
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jul 8, 2019

This JS would cause the problem being warned about:

document.title = 'original title';
test();
document.title = 'surprise!';

In other words, the value you're clobbering (document.title) wouldn't be "original title", it would be "surprise!" - that's a potential source of bugs.

@Lonniebiz
Copy link
Author

Lonniebiz commented Jul 9, 2019

@ljharb : document.title is just a node in the DOM. Obviously, it can be changed by any function that has access to the DOM later within other functions (this is by design). Are you advocating that estlint should error anytime an async-function modifies a DOM node or any variable outside of that function (even when there is no other instance of that value being modified in the entire rest of the program)?

Of course not! Therefore, this is a bug.

Why doesn't eslint also produce a race condition error if document.title is changed to window.document.title? Would it no less be the same problem you're describing?

For example, here, esilint does NOT produce the race condition error:

async function test()
{
	document.createElement("div");
	await Promise.resolve();
	window.document.title = "Test";
}
test();

Also, if you leave document.title alone (as original), but remove the document.createElement("div") line, this too does NOT produce a race condition error :

async function test()
{
	await Promise.resolve();
	document.title = "Test";
}
test();

Keep in mind, that the entire program is only those 7 lines you see in the first code segment of my original post. Where in those 7 lines do you see two things racing to set a value?

I agree that the function I posted is not pure (from a functional programming perspective), but there is no race condition. And, no error should occur; this is a bug.

@platinumazure
Copy link
Member

I have to agree, I'm not sure why this is anything but a bug. document.title is never read, and the reference to document is never modified. And the value assigned to document.title is a constant. If document.title were read anywhere, then we have a race condition for sure. But I could be missing something.

As for document vs window.document: In general, ESLint is not aware of the global object (window in browsers, global in Node, etc.) due to the need to be runtime-agnostic. So I wouldn't read too much into that if I were you.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jul 9, 2019
@mysticatea
Copy link
Member

Thank you for your report.

I think this is a duplicate of #11899. Please track that issue.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 6, 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 Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

4 participants