Add CoerceShared field-wise reborrow WF checks#157489
Conversation
Localize the WF helper logic to builtin coherence and add the directly related UI coverage.
rustbot
added
S-waiting-on-review
|
@rustbot label +F-Reborrow |
There was a problem hiding this comment.
Some issues but overall this looks pretty promising.
I'd kind of like the tests to all rather be added in the #157490 and then this PR to show which ones it fixes and which ones remain issues, but I'm not too partial on that point. I've reviewed the PR in the conventional comments style, so anything marked "thought" or "note" is just musings and not something you need to necessarily react to (unless you wish to), "question" is me being unsure but not something that would block the PR from merging, and "issues" and anything else marked "blocking" must be addressed before this can be considered for merging.
I'm also somewhat surprised that this PR didn't actually change any of the existing test results... I guess that's just a sign of our existing ui feature tests being insufficient :D
|
|
||
| // FIXME(#155345): This should return `Unnormalized` | ||
| fn collect_struct_data_fields<'tcx>( | ||
| #[derive(Clone, Copy, Debug)] |
There was a problem hiding this comment.
nitpick: If some of these types don't use the Debug impl we could remove it to save a bit of code generation.
| } | ||
|
|
||
| fn single_region_arg<'tcx>(args: ty::GenericArgsRef<'tcx>) -> Option<ty::Region<'tcx>> { | ||
| let mut lifetimes = args.iter().filter_map(|arg| arg.as_region()); |
There was a problem hiding this comment.
note: The region arguments are always at the beginning, so just doing args.first().is_some_and(|arg| arg.as_region().is_some()) or something like that would be equivalent.
| lifetimes.next().is_none().then_some(lifetime) | ||
| } | ||
|
|
||
| fn wf_data_fields<'tcx>( |
There was a problem hiding this comment.
nitpick: I'd like this function name to mention collecting, and I'm not sure "wf" is correct naming here since we don't really know that the field is well-formed at this point yet.
| } | ||
|
|
||
| // This is a coherence/WF check only. It verifies that the impl describes a | ||
| // structurally valid field relation; later runtime lowering still starts from |
There was a problem hiding this comment.
nitpick: Future readers won't know what "the signle reborrow adjustment" is (and that thing might not exist at some later point in time, while this wf-check might still remain). Let's maybe reword this to something like
This is a coherence/WF check only. It verifies that the CoerceShared impl describes a structurally valid field-wise relation. Runtime lowering of the operation is not modeled here.
| if by_position { | ||
| target_fields | ||
| .into_iter() | ||
| .zip(source_fields.into_iter().map(Some).chain(std::iter::repeat(None))) |
There was a problem hiding this comment.
thought: Huh... I somehow thought zipping iterators of different lengths would crash but that doesn't seem to be the case. Nice. Maybe I was thinking of zipping vectors?
| @@ -0,0 +1,23 @@ | |||
| //@ normalize-stderr: "\n\n\z" -> "\n" | |||
There was a problem hiding this comment.
nitpick: This does not appear anywhere in the code base currently. Normally this normalisation is used to deal with various platform-dependent outputs and random sources. This sort of "let's make the stderr file a little smaller in size" seems like an unnecessary complication.
|
|
||
| // SAFETY invariant: the pointer is valid as `&'a i32`. | ||
| #[derive(Clone, Copy)] | ||
| pub struct ForeignPtrRef<'a>((*const i32, PhantomData<&'a ()>)); |
There was a problem hiding this comment.
nitpick: The two types are subtly different to one another for no particular reason.
| //~^ ERROR | ||
|
|
||
| fn main() { | ||
| let local = LocalPtrMut((ptr::null(), PhantomData)); |
There was a problem hiding this comment.
issue: This code here is not related to what is actually being tested here AFAIU. The LocalMut/ForeignRef test above does not have anything in main.
| | | ||
| LL | let a = CustomMarker(PhantomData); | ||
| | - binding `a` declared here | ||
| LL | method(a); |
There was a problem hiding this comment.
thought: Huh... this seems confusing to me: if we've given an error from the CoerceShared implementation then I'd have assumed that here we'd get an error that CustomMarker does not match type StaticMarkerRef, ie. no adjustment should happen because CustomMarker should not implement CoerceShared...
| @@ -0,0 +1,25 @@ | |||
| //@ run-pass | |||
There was a problem hiding this comment.
question (blocking): This seems to be just a copy of the custom_marker_coerce_shared_copy.rs test - is there any difference?
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
3 participants
This PR attempts to add a well-formedness check for CoerceShared.
Split out of #157101
r? @aapoalas