misc coercion cleanups and handle safety correctly #148602
Conversation
- leak checking the lub for fndef<->fndef coerce-lubs - start lubbing closure<->closure coerce-lubs and leak check it
rustbot
added
S-waiting-on-author
BoxyUwU
changed the title
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
coercion_cleanup_uncontroversial
branch
from
bc29ba9 to
d3e3eaa
Compare
BoxyUwU
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred to the CTFE machinery Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a This PR changes a file inside Some changes occurred to constck cc @fee1-dead |
|
Hi @rust-lang/types. I've recently gone over our implementation of coercions and discovered a few things which I'd like to change which need to go through an FCP. I'm not really sure if it makes sense to split this into three separate FCPs or not so I've just kept it all together. Let me know if you'd rather me split them apart :) I am not including lang on this FCP as this is either clear bug fixes that make our behaviour more consistent and agree with what is already in the reference, or its just incredibly minor type checking stuff :) @rfcbot merge Leak Checks in
|
|
Team member @BoxyUwU has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
proposed-final-comment-period
| /// Go from a fn-item type to a fn pointer or an unsafe fn pointer. | ||
| /// It cannot convert an unsafe fn-item to a safe fn pointer. | ||
| ReifyFnPointer(hir::Safety), |
There was a problem hiding this comment.
I've gone and allowed ReifyFnPointer coercions to coerce safe fndefs to unsafe fn pointers directly instead of needing to compose two coercions. This made handling safety in coerce-lub simpler/nicer.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@rfcbot reviewed |
rust-rfcbot
added
the
final-comment-period
rust-rfcbot
removed
the
proposed-final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
| } | ||
| ty::Ref(r_b, _, mutbl_b) => { | ||
| return self.coerce_borrowed_pointer(a, b, r_b, mutbl_b); | ||
| return self.coerce_to_ref(a, b, mutbl_b, r_b); |
There was a problem hiding this comment.
why switch args? 🤔 feels like the previous order matched the order used in the ty itself?
| let target_ty = self.use_lub.then(|| self.next_ty_var(self.cause.span)).unwrap_or(b); | ||
|
|
||
| push_coerce_obligation(a, target_ty); | ||
| if self.use_lub { | ||
| push_coerce_obligation(b, target_ty); | ||
| } |
There was a problem hiding this comment.
let target_ty = if self.use_lub {
// When computing the lub, we create a new target
// and coerce both `a` and `b` to it.
let target_ty = self.next_ty_var(self.cause.span);
push_coerce_obligation(a, target_ty);
push_coerce_obligation(b, target_ty);
target_ty
} else {
// When subtyping, we don't need to create a new target
// as we only coerce `a` to `b`.
push_coerce_obligation(a, b);
b
};your code felt a bit unclear to me initially
| let ty::Ref(..) = coerced_a.kind() else { | ||
| span_bug!(self.cause.span, "expected a ref type, got {:?}", coerced_a); | ||
| }; |
There was a problem hiding this comment.
convert this to an assert_matches!/assert!(matches!(..))?
| // FIXME: we shouldn't be normalizing here as coercion is inside of | ||
| // a probe. This can probably cause ICEs. |
There was a problem hiding this comment.
I don't understand that FIXME. Normalizing inside of a probe is totally fine as long as we don't leak the infer vars. This happens for errors of commit_if_ok but I don't think not normalizing is the solution here.
More specifically, we have to normalize here as the fn-sig is the unnnormalized one from a function definition
| // We would then see that `FnDef(f)` can't be coerced to `Box<fn(?1)>` | ||
| // and return a `TypeError` referencing this new variable `?1`. This | ||
| // then caused ICEs as diagnostics would encounter inferences variables | ||
| // from the result of normalization inside of the probe used be coercion. |
|
|
||
| enum LeakCheck { | ||
| Yes, | ||
| Default, |
There was a problem hiding this comment.
Default seems weird. Maybe
/// Whether to explicitly leak check in `Coerce::unify_raw`.
///
/// FIMXE: We may want to change type relations to always leak-check
/// after exiting a binder, at which point we will always do so and
/// no longer need to handle this explicitly
enum ExplicitLeakCheck {
Yes,
No,
}| // Don't coerce pairs of fndefs or pairs of closures to fn ptrs | ||
| // if they can just be lubbed. | ||
| // | ||
| // See #88097 or `lub_closures_before_fnptr_coercion.rs` for where | ||
| // we would erroneously coerce closures to fnptrs when attempting to | ||
| // coerce a closure to itself. |
There was a problem hiding this comment.
maybe move this comment above let lubbed_tys. Right now this is kinda confusing. It not immediately clear why we have a closure without arguments here.
Or alternatively, maybe explicitly pass in prev_ty and new_ty to it 🤔
| // These don't currently lub but could in theory one day. | ||
| // If that happens this test should be adjusted to use | ||
| // fn ptrs that can't be lub'd. |
There was a problem hiding this comment.
hmm what? 🤔
- "fn ptrs that can't be lub'd" u mean fn defs?
- why can they in theory be lubbed at some point? their arg is invariant so there's no way to lub them, is there?
| // of an opaque with a hidden type of `Closure<C1, C2>`. This then allows | ||
| // us to substitute `C1` and `C2` for arbitrary types in the parent scope. | ||
| // | ||
| // See: <https://github.com/lcnr/random-rust-snippets/issues/13> |
There was a problem hiding this comment.
i think this test is cool given the amount of jank necessary to get it to work and it felt really good to get this to work.
I don't think it's useful for our UI suite however and will likely be annoying to handle if its behavior ever changes (which feels likely) 🤔
I would prefer to just have that snippet somewhere else, e.g. add it to lcnr/random-rust-snippets#13 or sth, instead of keeping it in our ui test suite
5 participants
r? lcnr
"remove normalize call"
Fixes #132765
If the normalization fails we would sometimes get a
TypeErrorcontaining inference variables created inside of the probe used by coercion. These would then get leaked out causing ICEs in diagnostics logic"leak check and lub for closure<->closure coerce-lubs of same defids"
Fixes rust-lang/trait-system-refactor-initiative#233
the
|x| x + 1expr has a type ofClosure(?31t)which we wind up inferring the RPIT to. TheCoerceManyret_coercionfor the wholepeculiartypeck has an expected type ofRPIT(unnormalized). When we type check thereturn |x| x + 1expr we go from the never type toClosure(?31t)which then participates in theret_coerciongiving us acoerce-lub(RPIT, Closure(?31t)).Normalizing
RPITgives us someClosure(?50t)where?31tand?50thave been unified with?31tas the root var.resolve_vars_if_possibledoesn't resolve infer vars to their roots so these wind up with different structural identities so the fast path doesn't apply and we fall back to coercing to afnptr. cc #147193 which also fixes thisNew solver probably just gets more inference variables here because canonicalization + generally different approach to normalization of opaques. Idk :3
FCP worthy stuffy
there are some other FCP worthy things but they're in my FCP comment which also contains some analysis of the breaking nature of the previously listed changes in this PR: #148602 (comment)