-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix AnonConst ICE #91312
Fix AnonConst ICE #91312
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
| Node::TraitRef(&TraitRef { path, .. }) => &*path, | ||
| Node::TraitRef(&TraitRef { path, .. }) => { | ||
// Check for empty generic args, because if there are no generic args, this would otherwise ICE | ||
if path.segments.len() == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if == 1
is correct here or if checking for < 2
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for the length of the segment does prevent the unwrap
triggered ICE in this test case, but this doesn't seem like the best way to prevent similar ICEs. I think something like 0: u8<e::f<5>=e::f>
should also trigger an ICE after your fixes.
Obviously this fuzzer generated example is really contrived, but to prevent this type of ICE, the most reasonable change to me seems to be to shift the code here, which tries to find the arg_index
and segment
from the path, to this pattern arm and not call unwrap
on the result, but just return None if this fails to find any arg_index
. Maybe try to minimize code duplication here.
I'm not sure about the other pattern arms, maybe calling unwrap
on the resulting path for those branches could also ICE and maybe we should never call unwrap
in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all thanks for the feedback, i'll look into applying the changes you suggested.
(btw 0: u8<e::f<5>=e::f>
doesn't panic at all neither on stable nor with my changed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about 0: u8<e<5>=e::f>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stable panics, with my changes it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mean that without knowing more about this code that it makes more sense to assume that the person who inserted that unwrap
probably had a good reason for including it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, i looked into it and i didn't find any case in which it could cause problems
Disclaimer: I am relatively new to rustc development, so maybe i missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jackh726 what are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so I guess in this case u8<e<5>=e>
is treated as type ascription for the 0
?
I think the right thing here is to change the unwrap_or_else(|| bug!())
here to a delay_span_bug
("no arg matching AnonConst in path") and a return None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so i applied your suggestions, I had to add another variable cuz i can't return the None
value in the unwrap_or_else
, also the test output didn't change
| Node::TraitRef(&TraitRef { path, .. }) => { | ||
// Check for empty generic args, because if there are no generic args, this would otherwise ICE | ||
if path.segments.len() == 1 | ||
&& path.segments[0].args.map_or(-1, |x| x.args.len() as isize) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if None
should be handled the same as len == 0
.
Also I am not sure if this is the correct way or if I should replace the |
| Node::TraitRef(&TraitRef { path, .. }) => &*path, | ||
| Node::TraitRef(&TraitRef { path, .. }) => { | ||
// Check for empty generic args, because if there are no generic args, this would otherwise ICE | ||
if path.segments.len() == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so I guess in this case u8<e<5>=e>
is treated as type ascription for the 0
?
I think the right thing here is to change the unwrap_or_else(|| bug!())
here to a delay_span_bug
("no arg matching AnonConst in path") and a return None
.
.delay_span_bug(tcx.def_span(def_id), "no arg matching AnonConst in path"); | ||
return None; | ||
} | ||
let (arg_index, segment) = filtered.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use a match with the above condition too?
This comment has been minimized.
This comment has been minimized.
When you fix formatting, can you also squash commits? |
Add test Apply suggestions Switch to match Apply cargofmt
4bf421a
to
a0fb992
Compare
Done |
@bors r+ rollup |
📌 Commit a0fb992 has been approved by |
…kh726 Fix AnonConst ICE I am not sure if this is even the correct place to fix this issue, but i went down the path where the generic args came from and i wasn't able to find a clear cause for this down there. But if anybody has a suggestion what i should do, just tell me. This fixes: rust-lang#91267
…kh726 Fix AnonConst ICE I am not sure if this is even the correct place to fix this issue, but i went down the path where the generic args came from and i wasn't able to find a clear cause for this down there. But if anybody has a suggestion what i should do, just tell me. This fixes: rust-lang#91267
…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#87614 (Recommend fix `count()` -> `len()` on slices) - rust-lang#91065 (Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE) - rust-lang#91312 (Fix AnonConst ICE) - rust-lang#91341 (Add `array::IntoIter::{empty, from_raw_parts}`) - rust-lang#91493 (Remove a dead code path.) - rust-lang#91503 (Tweak "call this function" suggestion to have smaller span) - rust-lang#91547 (Suggest try_reserve in try_reserve_exact) - rust-lang#91562 (Pretty print async block without redundant space) - rust-lang#91620 (Update books) - rust-lang#91622 (:arrow_up: rust-analyzer) Failed merges: - rust-lang#91571 (Remove unneeded access to pretty printer's `s` field in favor of deref) r? `@ghost` `@rustbot` modify labels: rollup
I am not sure if this is even the correct place to fix this issue, but i went down the path where the generic args came from and i wasn't able to find a clear cause for this down there. But if anybody has a suggestion what i should do, just tell me.
This fixes: #91267