Skip to content

Fix computed checks in acorn-optimizer. NFC #24271

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

Merged
merged 1 commit into from
May 7, 2025

Conversation

RReverser
Copy link
Collaborator

@RReverser RReverser commented May 6, 2025

.computed is a property of MemberExpression, but in a few cases it was incorrectly checked on CallExpressions.

This doesn't cause issues in existing tests, mostly noticed as a drive-by fix.

@RReverser RReverser requested a review from sbc100 May 6, 2025 21:14
@sbc100
Copy link
Collaborator

sbc100 commented May 6, 2025

How hard would it be to write a test for this?

@RReverser
Copy link
Collaborator Author

How hard would it be to write a test for this?

Hard enough that I don't even want to start thinking about it 😅

@RReverser RReverser enabled auto-merge (squash) May 6, 2025 22:27
@sbc100
Copy link
Collaborator

sbc100 commented May 6, 2025

How hard would it be to write a test for this?

Hard enough that I don't even want to start thinking about it 😅

Because constructing some JS to tickle this would be hard or because of unfamiliarity with the unit tests we have for JSDCE?

@RReverser
Copy link
Collaborator Author

RReverser commented May 6, 2025

Because constructing some JS to tickle this would be hard

This one. In theory, it might be doable with enough effort, but for now I'm focused on some larger changes and don't want to dig into this potential edge cases that no one even ran into so far.

@sbc100
Copy link
Collaborator

sbc100 commented May 6, 2025

Fair enough lgtm!

`.computed` is a property of `MemberExpression`, but in a few cases it was incorrectly checked on `CallExpression`s.
@RReverser RReverser merged commit ed115c5 into emscripten-core:main May 7, 2025
28 checks passed
@RReverser RReverser deleted the acorn-fixups branch May 7, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants