-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add suggestion for relaxing static lifetime bounds on dyn trait impls in NLL #97284
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
Add suggestion for relaxing static lifetime bounds on dyn trait impls in NLL #97284
Conversation
This comment has been minimized.
This comment has been minimized.
50e9b6d
to
b70c418
Compare
This comment has been minimized.
This comment has been minimized.
b70c418
to
c1c94a0
Compare
Weird that this compiled locally for me with the static_assert_size of 24. Probably needs a perf-run. |
I would have expected the NLL output of |
60aac63
to
1fda1fb
Compare
This comment has been minimized.
This comment has been minimized.
1fda1fb
to
81e0e09
Compare
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.
Still need to look at this a bit more, but was just thinking if there was any way to unify these suggestions with the existing ones?
ty::ReVar(..) => { | ||
bug!("StableHasher: unexpected region {:?}", *self) | ||
ty::ReVar(reg_vid) => { | ||
reg_vid.hash_stable(hcx, hasher); |
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.
Why did this change?
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.
Was hitting an ICE because ConstraintCategory
is HashStable
and we encounter ReVar
in the substs
that we added.
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 we add a delay_span_bug
call here? We really don't want to accidentally produce a binary if it shouldn'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.
Wouldn't we still ICE if we added that here (due to delay_span_bug being called, but no error having been found)?
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.
Yes, but only if no other errors are emitted, which would only happen if a diagnostic isn't built. I'm assuming that ReVar
is only being encountered due to diagnostic changes and not changes that would affect the final binary, right?
// Make sure this enum doesn't unintentionally grow | ||
rustc_data_structures::static_assert_size!(ConstraintCategory, 12); | ||
|
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.
Why remove the assert? (Are there problems when there is a lifetime?)
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.
Because SubstsRef
doesn't have a static size I think, at least I was hitting that assertion.
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.
Eh that's nonsense, of course it must have a static size. I really don't know why I was hitting those assertions, but the first two CI runs failed because of that, didn't have that problem locally.
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.
Two things: you might want to rebase on top of a recent master to see if that way you can replicate the issue locally, and you might want to turn the CallArgument
variant to hold a Box<Option<(DefId, SubstsRef<'tcx>)>>
to see if that keeps you under the 12 bytes size. Increasing the size of these types that are used a lot can cause compilation performance regressions that are really difficult to regain.
@@ -51,9 +52,19 @@ pub fn find_param_with_region<'tcx>( | |||
let hir_id = hir.local_def_id_to_hir_id(id.as_local()?); | |||
let body_id = hir.maybe_body_owned_by(hir_id)?; | |||
let body = hir.body(body_id); | |||
|
|||
// Don't perform this on closures |
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.
why?
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.
Because we can't call fn_sig
on closures.
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.
In the resulting ICE if you call fn_sig
on closures, it tells you what you should call instead: substs.as_closure().sig()
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 don't think it actually makes sense to look for closures here given the function's intention. This was previously never called on closures, it's just that the way we can call this we can still end up with a closure in this call and we need the HIR check to properly reject this if I remember correctly.
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 suspect that this might have been what caused the output changes that used to point at async
and closures (but don't know for sure and the output changes aren't too dramatic).
if let Some((ident, self_ty)) = | ||
self.get_impl_ident_and_self_ty_from_trait(instance.def_id(), &visitor.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.
Could use let else
☔ The latest upstream changes (presumably #96098) made this pull request unmergeable. Please resolve the merge conflicts. |
let call_arg = if let TerminatorKind::Call { func, .. } = &term.kind { | ||
let func_ty = func.ty(body, self.infcx.tcx); | ||
if let ty::FnDef(fn_did, substs) = func_ty.kind() { | ||
Some((*fn_did, *substs)) | ||
} else { | ||
None | ||
} | ||
} else { | ||
None | ||
}; | ||
debug!(?call_arg); |
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'm guessing the other cases that are not handled here are async
blocks and closures, right?
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.
Yes I believe so.
@@ -256,6 +263,70 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { | |||
outlives_suggestion.add_suggestion(self); | |||
} | |||
|
|||
fn get_impl_ident_and_self_ty_from_trait( |
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.
It feels to me like we might already have code doing this somewhere 🤔
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.
We do already have that functionality in lexical region diagnostics (NiceRegionError
), but can't directly use that struct in NLL (and at some point we will get rid of it anyway afaict).
for span in &traits { | ||
let mut multi_span: MultiSpan = vec![*span].into(); | ||
multi_span.push_span_label( | ||
*span, | ||
"this has an implicit `'static` lifetime requirement".to_string(), | ||
); | ||
multi_span.push_span_label( | ||
ident.span, | ||
"calling this method introduces the `impl`'s 'static` requirement".to_string(), | ||
); | ||
err.span_note(multi_span, "the used `impl` has a `'static` requirement"); | ||
err.span_suggestion_verbose( | ||
span.shrink_to_hi(), | ||
"consider relaxing the implicit `'static` requirement", | ||
" + '_".to_string(), | ||
Applicability::MaybeIncorrect, | ||
); | ||
suggested = true; | ||
} |
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 also think that we might already have logic for this, but if it is not accessible for some reason, I'm not above a little duplication for a clear diagnostic improvement.
LL | let cell = Cell::new(&a); | ||
| ^^ borrowed value does not live long enough | ||
... | ||
LL | } | ||
| - `a` dropped here while still borrowed | ||
LL | / foo(cell, |cell_a, cell_x| { | ||
LL | | cell_x.set(cell_a.get()); // forces 'a: 'x, implies 'a = 'static -> borrow error | ||
LL | | }) | ||
| |______- argument requires that `a` is borrowed for `'static` | ||
LL | } | ||
| - `a` dropped here while still borrowed |
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'm somewhat worried about the output changes here and in the async
block in /issue-72312.nll.stderr
. It also seems like we should also special case their output if we could 🤔
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.
This case actually seems like an improvement, we "just" need to figure out how to point at the inside of the closure :-/
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.
It's kind of weird that this does not point to the start of the closure anymore. Personally I don't really think this makes the diagnostic any less helpful (since the point where the drop happen implies that anyway), but I can try to investigate if you find this important.
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.
Would you mind to do so as a follow up? In this case in particular it is an improvement, but I'm not so sure about the other ones.
e04af21
to
5422fea
Compare
This comment has been minimized.
This comment has been minimized.
5422fea
to
d3be764
Compare
@estebank Addressed your review. We use the |
This comment has been minimized.
This comment has been minimized.
📌 Commit 3c6c8d5 has been approved by |
⌛ Testing commit 3c6c8d5 with merge 2a0c6ac83a9a29409d6db4d5f2d77457261993ce... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
ty::ReVar(..) => { | ||
bug!("StableHasher: unexpected region {:?}", *self) | ||
ty::ReVar(reg) => { | ||
reg.hash_stable(hcx, hasher); |
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 wonder if the LLVM failure is due to this. As a refresher, what are the failures when you keep the bug!
?
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.
Don't think it's related to that. ReVar
is HashStable
, we just previously had the bug!
call in there, because it wasn't used when hashing Region
afaict. I just got the output of the message in the bug statement.
I'm sort of leaning towards thinking that this failure is spurious. The test is in the ui tests and passed on all other platforms and the code changes in this PR don't touch anything that should cause different behaviour on different platforms.
@bors retry maybe spurious? |
⌛ Testing commit 3c6c8d5 with merge 6c557271b8989188a4d0dbb93ad2a7fb1fbe5626... |
💔 Test failed - checks-actions |
@bors retry spurious network error |
☀️ Test successful - checks-actions |
Finished benchmarking commit (ed76b77): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Visiting for weekly performance triage.
|
This PR introduces suggestions for relaxing static lifetime bounds on impls of dyn trait items for NLL similar to what is already available in lexical region diagnostics.
Fixes #95701
r? @estebank