Skip to content

Comments

fix(semantic): consider update expressions in implicit returns as reads#5158

Closed
DonIsaac wants to merge 1 commit intomainfrom
don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads
Closed

fix(semantic): consider update expressions in implicit returns as reads#5158
DonIsaac wants to merge 1 commit intomainfrom
don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads

Conversation

@DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Aug 23, 2024 •
edited
Loading

Closes #5156

@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 23, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@DonIsaac Graphite App
Copy link
Contributor Author

DonIsaac commented Aug 23, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-ast Area - AST labels Aug 23, 2024
@DonIsaac DonIsaac marked this pull request as ready for review August 23, 2024 23:34
@DonIsaac DonIsaac mentioned this pull request Aug 23, 2024
Merged
@DonIsaac DonIsaac added C-bug Category - Bug blocker labels Aug 23, 2024 — with Graphite App
@DonIsaac DonIsaac requested a review from Dunqing August 23, 2024 23:34
@DonIsaac DonIsaac linked an issue Aug 23, 2024 that may be closed by this pull request
bug(semantic): implicit return in arrow function does not count as a read #5156
Closed
@DonIsaac DonIsaac force-pushed the don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads branch 2 times, most recently from c9f7308 to 70f30e6 Compare August 23, 2024 23:38
@DonIsaac DonIsaac requested a review from overlookmotel August 23, 2024 23:39
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 23, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #5158 will not alter performance

Comparing don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads (5a4faf1) with main (d29042e)

Summary

✅ 29 untouched benchmarks

@DonIsaac DonIsaac force-pushed the don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads branch from 70f30e6 to 20ebc2e Compare August 24, 2024 01:18
Dunqing
Dunqing requested changes Aug 24, 2024
View reviewed changes
crates/oxc_semantic/src/builder.rs
Comment on lines +1235 to +1237
let in_return = self.is_in_return;
self.is_in_return = true;
self.visit_expression(arg);
self.is_in_return = in_return;
Copy link
Member

@Dunqing Dunqing Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause a bit of difficulty in maintaining. We should avoid writing any logic in the visit function.

@Dunqing Dunqing mentioned this pull request Aug 24, 2024
Merged
@Dunqing
Copy link
Member

Dunqing commented Aug 24, 2024

I tried another way to fix this in #5161. Thank you very much for adding so many tests! I have copied them into #5161. If we approve #5161, you can create a separate PR to update the documentation and fix the false positive for linter

@DonIsaac
Copy link
Contributor Author

DonIsaac commented Aug 24, 2024

@Dunqing I'll split the linter test cases into a separate PR, I think I accidentally committed them... there's a few test cases missing in this PR, which I've left as comments in yours.

overlookmotel
overlookmotel reviewed Aug 24, 2024
View reviewed changes
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite hard to give an opinion on. Personally I don't understand why i++ / ++i isn't always considered Read | Write (#3326), so I don't know what the meaning of the Read flag is currently.

But, putting that aside for a minute and assuming that your logic is correct... in terms of implementation, is it not possible to use current_reference_flags for this, rather than introducing a new is_in_return property?

crates/oxc_semantic/tests/integration/symbols.rs
Comment on lines +385 to +389
SemanticTester::js("let i = 0; const x = () => (++i, 0)")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(1)
.test();
Copy link
Member

@overlookmotel overlookmotel Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test correct? I don't think i is being read from here (but see my general comment - I'm not sure).

Copy link
Member

@Dunqing Dunqing Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I am not sure what i ReferenceFlags should be. The x function return result is 0 when it is called, so i should be write only. I think we can try to make i only contain write if no tests are broken

Copy link
Member

@overlookmotel overlookmotel Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to get to the bottom of it! #5165

Copy link
Contributor Author

@DonIsaac DonIsaac Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's not, but there are other test cases that enforce this behavior. I mentioned this to you (@Dunqing) a while ago, when you made that change, and your rationale was that "we don't know what's a read and what's not a read, since things could be proxies and/or getters/setters"

@overlookmotel
Copy link
Member

overlookmotel commented Aug 24, 2024

But, putting that aside for a minute and assuming that your logic is correct... in terms of implementation, is it not possible to use current_reference_flags for this, rather than introducing a new is_in_return property?

Oh, I now see Dunqing has taken exactly this approach in #5161...

@overlookmotel overlookmotel mentioned this pull request Aug 24, 2024
Closed
@Boshen Boshen changed the base branch from don/08-23-fix_linter/no-unused-vars_function_expression_in_implicit_arrow_function_return to graphite-base/5158 August 24, 2024 15:49
@Boshen Boshen force-pushed the don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads branch from 20ebc2e to 811440b Compare August 24, 2024 15:54
@Boshen Boshen force-pushed the graphite-base/5158 branch from 8833061 to d29042e Compare August 24, 2024 15:54
@Boshen Boshen changed the base branch from graphite-base/5158 to main August 24, 2024 15:55
@Boshen Boshen force-pushed the don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads branch from 811440b to 5a4faf1 Compare August 24, 2024 15:55
@graphite-app graphite-app bot closed this in 293413f Aug 26, 2024
@Boshen Boshen deleted the don/08-23-fix_semantic_consider_update_expressions_in_implicit_returns_as_reads branch September 11, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overlookmotel overlookmotel overlookmotel left review comments

@Dunqing Dunqing Dunqing requested changes

Assignees

No one assigned

Labels

A-ast Area - AST A-linter Area - Linter A-semantic Area - Semantic blocker C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(semantic): implicit return in arrow function does not count as a read

3 participants

@DonIsaac @Dunqing @overlookmotel