Do proper signature equality checking for tail calls#156007
Do proper signature equality checking for tail calls#156007WaffleLapkin wants to merge 7 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
| become foo(x, y); | ||
| //~^ ERROR mismatched signatures | ||
| //[current]~^ ERROR mismatched signatures | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not quite sure (lcnr can probably explain better), but something something opaque types are more annoying to handle in the old solver and in this case they are not being revealed as their hidden type.
There was a problem hiding this comment.
the main issue is that the old solver does not have a way to say "we're in the defining scope of the following opaque types and have already inferred their hidden types".
So while you can kind of hack around this by allowing them to be redefined and then checking that they match the actually inferred value, this is annoying and doesn't fully work. This is what check_opaque_type_well_formed (I think that's what the method is called) does
3371ceb to
0ef36d2
Compare
This comment has been minimized.
This comment has been minimized.
15bd832 to
1ce7add
Compare
1ce7add to
6623564
Compare
6623564 to
1ddb39e
Compare
This comment has been minimized.
This comment has been minimized.
1ddb39e to
cf7e816
Compare
| if let Err(ErrorGuaranteed { .. }) = self.found_errors { | ||
| // We do a lot of special-cased errors above; equating signatures below would duplicate | ||
| // them, so bail out if we emitted any errors so far. | ||
| return; |
There was a problem hiding this comment.
i feel like it would be a lot nicer for this to return Result<(), ErrorGuaranteed> to make the eager returns easier to reason about
There was a problem hiding this comment.
Tried this. Not sure if it's better or worse.
I've used ? as ! to signify "this always returns an error". Kinda goofy, but I think it's the best way to describe this idea.
| self.report_signature_mismatch( | ||
| expr.span, | ||
| self.tcx.liberate_late_bound_regions( | ||
| CRATE_DEF_ID.to_def_id(), |
There was a problem hiding this comment.
that error reporting is weird 🤔
why are you using the identity signature of the caller_ty I guess it's fine as we're in the caller. But then it's just instantiate_identity and liberate_late_bound_regions using the caller_def_id.
Also, why liberate instead of just instantiating with infer or sth 🤔
There was a problem hiding this comment.
liberate isn't wrong necessarily, just a bit weird if we don't care about regions
There was a problem hiding this comment.
I don't necessarily understand why liberate_late_bound_regions is used here. Got introduced in #145012 (comment)...
cf4857c to
27c59a3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
27c59a3 to
f184753
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| if self.is_closure { | ||
| self.report_in_closure(expr); | ||
| return; | ||
| self.report_in_closure(expr)? as !; |
There was a problem hiding this comment.
hmm i'd do return Err(self.report_in_closure(expr)); 😁
There was a problem hiding this comment.
or wait the reporting functions use Result<!, ErrorGuaranteed> instead of just ErrorGuaranteed 🤔
idk how I feel about that
There was a problem hiding this comment.
I guess my vibe is, seems worse than just returning ErrorGuaranteed directly and wrapping it in Err here
There was a problem hiding this comment.
I just really don't like return Err(...) as a construct tbh. But if you think it's clearer, I can rewrite this to just use return Err(...), it's trivial to do so...
There was a problem hiding this comment.
weak preference for that :x I do dislike self.report_in_closure(expr)? as !;
I also don't care enough to block things on bikeshed tbh, do what you want cause a pirate is free YOU ARE A PIRATE
|
☔ The latest upstream changes (presumably #158891) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
For tail calls to be valid, we need some support from the ABI, specifically the caller of the function needs to be able to do all necessary cleanup after the tail called function returns. This can be achieved with a callee-cleanup ABI, but we want to permit tail calls for other ABIs as well (in particular,
extern "rust").In such cases the property that we are looking for is "the caller and callee require the same cleanup". We approximate this with "caller and callee have the same ABI" and we approximate that with "caller and callee have the same signatures (modulo subtyping)".
Previously the code would compare
FnSigs with==, which would not support subtyping and produced other weird results. This PR changes tail call checks to use type checking machinery and in particulareq.It also fixes handling of opaque types, given the next trait solver is enabled.
r? types