fix: Port call expr type checking and closure upvar inference from rustc#22101
Conversation
…of the bindings inside Previously it was the closure, which is incorrect.
And not a DefWithBodyId.
rustbot
added
the
S-waiting-on-review
| #[test] | ||
| fn capture_specific_fields() { | ||
| size_and_align_expr! { | ||
| minicore: fn; |
There was a problem hiding this comment.
The new call implementation is sensitive to FnX in core; anything that call closures need them, or it'll panic.
| } | ||
| "#, | ||
| expect!["53..79;20..21;67..72 ByRef(Shared) *a &'? bool"], | ||
| expect!["53..79;20..21;62..72 ByRef(Immutable) *a &'<erased> bool"], |
There was a problem hiding this comment.
I have to admit I didn't check all the changed ranges here to make sense 😅 If the reviewer wants to do that, well..., that's not easy. But convert_closure_to_fn relies on the ranges and its test passes, and I did check the capture kinds.
| "#, | ||
| r#" | ||
| struct A { b: &'static B, c: i32 } | ||
| struct A { b: &'static mut B, c: i32 } |
There was a problem hiding this comment.
Here I changed it to mut because leaving it as is (there is a test down with the same structs I left as-is), the capture will be &B instead of &i32. This is due to truncate_capture_for_optimization(), which we didn't have earlier.
| ## Captures | ||
| * `x.f1` by move | ||
| * `(*x.f2.0.0).f` by mutable borrow | ||
| * `x.f2.0.0.f` by mutable borrow |
There was a problem hiding this comment.
This is due to a small change (in my opinion, and in the code): I decided that we don't need a separate display() for the place, and we can use display_for_source_code(). The difference between the two used to be that the non-source-code version always printed derefs, with parentheses if needed. I decided now that it's nicer to show without them, given that the user expects autoderef anyway. Note that in the closure_captures.rs tests we do print all derefs, to ensure better coverage.
| fn foo() { | ||
| let closure; | ||
| // ^^^^^^^ impl Fn() | ||
| // ^^^^^^^ impl FnOnce() |
There was a problem hiding this comment.
The functions in this test error in rustc, so whatever we infer is good enough.
ChayimFriedman2
force-pushed
the
port-infer-call
branch
from
3ccf7e6 to
b8d0e9d
Compare
| let res: serde_json::Value = serde_json::from_str(res.as_str().unwrap()).unwrap(); | ||
| let arr = res.as_array().unwrap(); | ||
| assert_eq!(arr.len(), 2); | ||
| assert_eq!(arr.len(), 1); |
There was a problem hiding this comment.
There previously was a duplicate goal.
ChayimFriedman2
force-pushed
the
port-infer-call
branch
3 times, most recently
from
3154dc6 to
cb0f840
Compare
|
This analysis_stats failure - turns out we're just missing match arm guards in |
ChayimFriedman2
force-pushed
the
port-infer-call
branch
5 times, most recently
from
a16543e to
37febba
Compare
| f(*expr); | ||
| arms.iter().for_each(|arm| { | ||
| f(arm.expr); | ||
| if let Some(guard) = arm.guard { |
There was a problem hiding this comment.
In these kinds of functions we should really enforce restructuring patterns to catch things 😅
There was a problem hiding this comment.
Agree, but it's better to leave that to another PR.
| @@ -1,3 +1,4 @@ | |||
| //- minicore: fn | |||
There was a problem hiding this comment.
Curious where we panic without language items, ideally that wouldn't happen (even if in practice no one will ever write a no_core rust program without the fn traits ...). Just looks a bit odd to add a mini core directive here haha
There was a problem hiding this comment.
I wonder if we should add this directive as enabled by default for tests now
There was a problem hiding this comment.
It panicked in the past as Expected to find a suitable `Fn`/`FnMut`/`FnOnce` implementation for ... in callee.rs, this was copied from rustc and I preferred not to change it. But I was forced to, because it panicked in the sysroot run in CI (it doesn't have a proper sysroot configured), and there I could not inject a minicore 😅
| Self::try_overloaded_call_step(call_expr, callee_expr, arg_exprs, &mut autoderef); | ||
| } | ||
|
|
||
| // FIXME: rustc does some ABI checks here, but the ABI mapping is in rustc_target and we don't have access to that crate. |
There was a problem hiding this comment.
Curious what mapping you are speaking of here 🤔
There was a problem hiding this comment.
| // FIXME: remove this field | ||
| pub mutated_bindings_in_closure: FxHashSet<BindingId>, |
There was a problem hiding this comment.
ChayimFriedman2
force-pushed
the
port-infer-call
branch
from
37febba to
3d6be00
Compare
rustbot
removed
the
S-waiting-on-review
3 participants
The first commits are small.
The biggest differences from rustc are due to the fact that we need to precisely store all source spans for captures, for the convert_closure_to_fn assist.