Remove the empty variant of VerifyBound#152438
Open
amandasystems wants to merge 4 commits intorust-lang:mainfrom
Open
Remove the empty variant of VerifyBound#152438amandasystems wants to merge 4 commits intorust-lang:mainfrom
VerifyBound#152438amandasystems wants to merge 4 commits intorust-lang:mainfrom
Conversation
This variant is not encountered in region inference at all. Where it is used, it is replaced with an empty any (or) bound, which is checked for as a special case with the equivalent logic where necessary. Such a bound will always fail in region inference, so this should not introduce any soundness issues.
rustbot
added
S-waiting-on-review
Collaborator
|
rustbot has assigned @JonathanBrouwer. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Contributor
Author
lcnr
reviewed
Feb 12, 2026
lcnr
reviewed
Feb 12, 2026
Comment on lines
+955
to
963
| // An empty bound holds for an empty variable. | ||
| VerifyBound::AnyBound(bs) if bs.is_empty() => match min.kind() { | ||
| ty::ReVar(rid) => match var_values.values[rid] { | ||
| VarValue::ErrorValue => false, | ||
| VarValue::Empty(_) => true, | ||
| VarValue::Value(_) => false, | ||
| }, | ||
| _ => false, | ||
| }, |
Contributor
There was a problem hiding this comment.
That seems undesirable as it means we still have VerifyBound::IsEmpty, just represented differently. Why is this necessary?
lcnr
reviewed
Feb 12, 2026
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
|
r? lcnr |
lcnr
reviewed
Feb 14, 2026
Contributor
Author
I’m on it, but it seems interesting to me because as you and Boxy both said this shouldn’t happen so maybe I found something here. :) |
Contributor
Author
I think this is the same failure as the one I get for I'm looking into where that is coming from. |
Contributor
Author
|
It's apparently coming from I'm not sure if the information is available that would allow us to detect this case and simply not emit the bound in this case. |
Contributor
|
I expect |
Contributor
Author
I think so, the failing code looks like this: pub fn escaped<'a, I: 'a, Error, F, G, O1, O2>(
mut normal: F,
control_char: char,
mut escapable: G,
) -> impl Parser<I, <I as Stream>::Slice, Error>
where
I: StreamIsPartial,
I: Stream,
<I as Stream>::Token: AsChar + Clone,
F: Parser<I, O1, Error>,
G: Parser<I, O2, Error>,
Error: ParserError<I>,
{
trace("escaped", move |input: &mut I| {
if <I as StreamIsPartial>::is_partial_supported() && input.is_partial() {
streaming_escaped_internal(input, &mut normal, control_char, &mut escapable)
} else {
complete_escaped_internal(input, &mut normal, control_char, &mut escapable)
}
})
}I haven’t gotten around to minimising it yet but I suspect what trace does is just print something and in that case it shouldn’t be doing much interesting inside the closure where the failure is happening. I’ll look into it some more once I’m better from my migraine, probably Wednesday. |
Contributor
Author
Here's a reduction! pub fn escaped<'a, I: 'a, F, O1>(mut normal: F) -> impl Parser<I, <I as Stream>::Slice>
where
I: Stream,
F: Parser<I, O1>,
{
trace(move |_input: &mut I| uses_normal(&mut normal))
}
fn uses_normal<I, F, O1>(_normal: &mut F) -> <I as Stream>::Slice
where
I: Stream,
F: Parser<I, O1>,
{
todo!()
}
pub fn trace<I: Stream, O>(parser: impl Parser<I, O>) -> impl Parser<I, O> {
parser
}
impl<'a, I, O, F> Parser<I, O> for F
where
F: FnMut(&mut I) -> O + 'a,
I: Stream,
{
}
pub trait Stream {
type Slice;
}
pub trait Parser<I, O> {}What happens here is that Apparently, there are no default region bounds to apply in this case, I'm not familiar enough with this part of the compiler to know why. |
Contributor
|
does this error in MIR borrowck? |
21 tasks
Contributor
Author
Nope! It's emitted by trait selection as a generic bound failure. Here are the relevant parts: The failure seems to happen in In other words, there is an empty |
Contributor
|
Happening when validating that the opaque types inferred in borrowck are well-formed Unhappy about this 😅 this example should compile. I feel like the regression test could be made more minimal :3 I hate that we allow unconstrained lifetimes in impls in the first place :/ we should at least lint on them. Looking for the existing discussion here https://rust-lang.zulipchat.com/#narrow/channel/144729-t-types/topic/existing.20issue.20for.20unconstrained.20lifetimes.20in.20impls/with/581309540 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This variant is not encountered in region inference at all. Where it is used, it is replaced with an empty any (or) bound, which is checked for as a special case with the equivalent logic where necessary. Such a bound will always fail in region inference, so this should not introduce any soundness issues.