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

[DonIsaac]

Closes #5156

[graphite-app]

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]

• fix(semantic): consider update expressions in implicit returns as reads #5158 👈
• fix(linter/no-unused-vars): function expression in implicit arrow function return #5155
• main

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

Join @DonIsaac and the rest of your teammates on Graphite

[codspeed-hq]

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

[Dunqing]

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

[Dunqing]

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]

@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]

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?

[overlookmotel]

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

[Dunqing]

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

[overlookmotel]

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

[DonIsaac]

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]

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...