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

Guard against opaques crashing the compiler #19541

Closed
wants to merge 1 commit into from

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Jan 26, 2024

Fixes #19434

@odersky
Copy link
Contributor

odersky commented Jan 26, 2024

One possibility would be to merge ElimOpaque with erasure. It's a tiny phase. I am not sure anything between ElimOpaque and Erasure depends on the fact that opaque aliases are now aliases. So far no phase declares that it needs to run after ElimOpaque but that's of course no guarantee that there are no dependencies.

@dwijnand
Copy link
Member Author

I could see that fixing this case, seeing as the code branch is run in preErasureCtx.

@dwijnand dwijnand force-pushed the dont-make-non-opaque-crash branch 2 times, most recently from d7ad0c6 to c14daff Compare February 1, 2024 13:40
@dwijnand dwijnand changed the title Guard against elim opaque crashing the compiler Guard against opaques crashing the compiler Feb 1, 2024
@dwijnand
Copy link
Member Author

dwijnand commented Feb 6, 2024

How do we deal with SeqLiterals and opaque types? With merging ElimOpaque into Erasure, we now box opaque primitives. And I can't find a way to phase travel and get classSymbol to give me the Int symbol.

@odersky
Copy link
Contributor

odersky commented Feb 6, 2024

Hmm. Maybe we have to leave ElimOpaque where it is since it seems to upset quite a few assumptions afterwards. We should document in the relevant phases that they have to run after ElimOpaque.

That means that match type reduction has to cope with this somehow. One trick we could play is to refuse to normalize match types after ElimOpaque. Instead, time-travel to the ElimOpaque phase and do the reduction there. Or else treat normal aliases like opaque aliases, if the symbol has Opaque set before ElimOpaque. Not sure whether the second one is safe, though.

@dwijnand
Copy link
Member Author

dwijnand commented Feb 7, 2024

Instead, time-travel to the ElimOpaque phase and do the reduction there. Or else treat normal aliases like opaque aliases, if the symbol has Opaque set before ElimOpaque. Not sure whether the second one is safe, though.

I didn't quite understand what you meant here. Could you explain again what you think we should try? Currently I'm just phase travelling to typer.

@sjrd
Copy link
Member

sjrd commented Feb 8, 2024

My take on that idea is that we could try an ElimMatchTypes before ElimOpaues. It would replace every match type that reduces to its reduction, and for the others leave them as shells that only keep the upper bound and can never further reduce.

I'm not sure whether that's sound TBH.

@dwijnand
Copy link
Member Author

dwijnand commented Feb 9, 2024

Can't make that work. It fails Ycheck.

@dwijnand dwijnand marked this pull request as ready for review February 15, 2024 13:12
@dwijnand dwijnand assigned sjrd and unassigned dwijnand Feb 15, 2024
@dwijnand dwijnand closed this Apr 30, 2024
@dwijnand dwijnand deleted the dont-make-non-opaque-crash branch April 30, 2024 13:06
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.

Illegal MatchType Error reaches the backend
3 participants