-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
fixed ICE for more than one eii implementations #149985
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm not sure if this is just some temporarily placeholder with the intention that it will be eventually supported. If so, in that case, emitting some specific diagnostics doesn't feel necessary, the panic is fine. Not sure about the context, so maybe r? @jdonszelmann (if available) |
|
We likely cannot ever support this, so sure, it's probably alright to make this into a diagnostic not a panic. |
|
Could you please turn it into a structured diagnostic though like the other ones around here? |
|
Nooo, wait a second, stop, this isn't what's going on here. There's a deeper bug here. This diagnostic is I guess fine, though there will never be a stable way to achieve this since adding a diagnostic out separated from the declaration won't be something you'd normally do. The problem is that this won't fix #149982. That code shouldn't get here. Please give me a little bit of time researching that and then we can maybe move forward here. I don't want to accidentally hide an otherwise real bug. |
|
@rustbot blocked |
|
@jieyouxu I'm available for any review surrounding eiis, and probably atm the only one who should given how experimental this one is. If you see more like it feel free to assign me <3 |
|
Oh and this one will have to be squashed in any case fyi |
|
Thanks for the clarification. That makes sense. I agree it would be better not to mask a deeper issue. Happy to wait and help investigate once you have had time to look into it. |
Fixes #149982.
this pr fixes an ice issue which was there by having multiple default eii implementations. the compiler previously panicked in this situation but now it gives a clear error instead.
also, a ui test is added to cover this case and prevent any implications.