Skip to content

Remove questionable transform of Select from SelectStatic#25620

Open
SolalPirelli wants to merge 2 commits intoscala:mainfrom
dotty-staging:solal/24707
Open

Remove questionable transform of Select from SelectStatic#25620
SolalPirelli wants to merge 2 commits intoscala:mainfrom
dotty-staging:solal/24707

Conversation

@SolalPirelli
Copy link
Copy Markdown
Contributor

Fixes #24707

This transform changed { x ; y }.v to { x ; y.v }.
But this is not valid if it's within an assignment, e.g., { x ; y }.v = 42 becoming the invalid { x ; y.v } = 42.

How much have you relied on LLM-based tools in this contribution?

not

How was the solution tested?

regression test from the issue

@som-snytt
Copy link
Copy Markdown
Contributor

I'm motivated to learn, when I have a few minutes, why it's OK to revert the original code for the phase. I see it was to support the backend.

@SolalPirelli SolalPirelli marked this pull request as ready for review March 26, 2026 08:32
@SolalPirelli
Copy link
Copy Markdown
Contributor Author

@som-snytt it seems based on git blame that the backend used to emit incorrect code in such cases. I guess this is a classic case of a "temporary" workaround.

@sjrd
Copy link
Copy Markdown
Member

sjrd commented Apr 2, 2026

it seems based on git blame that the backend used to emit incorrect code in such cases. I guess this is a classic case of a "temporary" workaround.

If we suspect that the backend was producing bad code, we should have a run test to make sure the produced bytecode is validated by the JVM.

@SolalPirelli
Copy link
Copy Markdown
Contributor Author

i1442 added by the original SelectStatic PR still passes, but it's true it's not the greatest coverage https://github.com/scala/scala3/pull/1445/changes

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.

Compiler crash with "scala.MatchError: NoType"

3 participants