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

Incorrect reference flags for x++ and x-- #3326

Closed
overlookmotel opened this issue May 17, 2024 · 3 comments · Fixed by #3354
Closed

Incorrect reference flags for x++ and x-- #3326

overlookmotel opened this issue May 17, 2024 · 3 comments · Fixed by #3354
Assignees
Labels
C-bug Category - Bug

Comments

@overlookmotel
Copy link
Collaborator

overlookmotel commented May 17, 2024

let x;
x += 1;
x++;
++x;
x--;
--x;

x += 1 gets correctly flagged as Read | Write. But all the rest (x++ etc) are flagged as just Write. This appears to be incorrect.

playground

@Dunqing
Copy link
Member

Dunqing commented May 18, 2024

This seems to be intentional

// unary op counts as write, but checking continues up tree
(SourceType::default(), "let a = 1, b; b = ++a", ReferenceFlag::read_write()),
(SourceType::default(), "let a = 1, b; b = --a", ReferenceFlag::read_write()),
(SourceType::default(), "let a = 1, b; b = a++", ReferenceFlag::read_write()),
(SourceType::default(), "let a = 1, b; b = a--", ReferenceFlag::read_write()),

Same behavior in no_unused_private_class_members rule

r"class C {
#usedOnlyInIncrement;
foo() {
this.#usedOnlyInIncrement++;
}
}",

But I think x += 1 should be flagged as Write because x is never used for reading. I believe this variable can be safely removed by the Minifier/Compressor.

Does this make sense?

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented May 18, 2024

Hmm. I'm not clear what these flags are meant to represent now!

These are all (broadly) equivalent:

x = x + 1; // Read + Write
x += 1; // Read | Write
x++; // Write
++x; // Write

NB: Also depends on what x is. If it's an object with a valueOf method, the any of the above can have side effects.

This also seems to be inconsistent - it flags x as Read | Write where it could be just Write:

let x = 1;
let y = x = 2;

playground

And something strange going on here - the addition of console.log makes x Read | Write:

console.log;
let x = 1;
x++;

playground

@Dunqing
Copy link
Member

Dunqing commented May 19, 2024

Hmm. I'm not clear what these flags are meant to represent now!
These are all (broadly) equivalent:

x = x + 1; // Read + Write
x += 1; // Read | Write
x++; // Write
++x; // Write

The x = x + 1 is not equivalent. This assignment expression has two references. x + 1 will be flagged Read and x = will be flagged Write

NB: Also depends on what x is. If it's an object with a valueOf method, the any of the above can have side effects.

Yes! I forgot the valueOf method.

This also seems to be inconsistent - it flags x as Read | Write where it could be just Write:

let x = 1;
let y = x = 2;

playground

This case is correct. x = 2 should be flagged as Write and y = x should be flagged as Read

And something strange going on here - the addition of console.log makes x Read | Write:

console.log;
let x = 1;
x++;

playground

This is a bug! I will fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants