MIR sanity check: validate types on assignment #72796
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,11 @@ use rustc_middle::{ | |
| BasicBlock, Body, Location, Operand, Rvalue, Statement, StatementKind, Terminator, | ||
| TerminatorKind, | ||
| }, | ||
| ty::{ | ||
| self, | ||
| relate::{Relate, RelateResult, TypeRelation}, | ||
| ParamEnv, Ty, TyCtxt, | ||
| }, | ||
| }; | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
|
|
@@ -28,6 +32,93 @@ impl<'tcx> MirPass<'tcx> for Validator { | |
| } | ||
| } | ||
|
|
||
| /// Returns whether the two types are equal up to lifetimes. | ||
| /// All lifetimes, including higher-ranked ones, get ignored for this comparison. | ||
| /// (This is unlike the `erasing_regions` methods, which keep higher-ranked lifetimes for soundness reasons.) | ||
| /// | ||
| /// The point of this function is to approximate "equal up to subtyping". However, | ||
| /// the approximation is incorrect as variance is ignored. | ||
| pub fn equal_up_to_regions( | ||
| tcx: TyCtxt<'tcx>, | ||
| param_env: ParamEnv<'tcx>, | ||
| src: Ty<'tcx>, | ||
| dest: Ty<'tcx>, | ||
| ) -> bool { | ||
| struct LifetimeIgnoreRelation<'tcx> { | ||
| tcx: TyCtxt<'tcx>, | ||
| param_env: ty::ParamEnv<'tcx>, | ||
| } | ||
|
|
||
| impl TypeRelation<'tcx> for LifetimeIgnoreRelation<'tcx> { | ||
| fn tcx(&self) -> TyCtxt<'tcx> { | ||
| self.tcx | ||
| } | ||
|
|
||
| fn param_env(&self) -> ty::ParamEnv<'tcx> { | ||
| self.param_env | ||
| } | ||
|
|
||
| fn tag(&self) -> &'static str { | ||
| "librustc_mir::transform::validate" | ||
| } | ||
|
|
||
| fn a_is_expected(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn relate_with_variance<T: Relate<'tcx>>( | ||
| &mut self, | ||
| _: ty::Variance, | ||
| a: &T, | ||
| b: &T, | ||
| ) -> RelateResult<'tcx, T> { | ||
| // Ignore variance, require types to be exactly the same. | ||
| self.relate(a, b) | ||
| } | ||
|
|
||
| fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> { | ||
| if a == b { | ||
| // Short-circuit. | ||
| return Ok(a); | ||
| } | ||
| ty::relate::super_relate_tys(self, a, b) | ||
| } | ||
|
|
||
| fn regions( | ||
| &mut self, | ||
| a: ty::Region<'tcx>, | ||
| _b: ty::Region<'tcx>, | ||
| ) -> RelateResult<'tcx, ty::Region<'tcx>> { | ||
| // Ignore regions. | ||
| Ok(a) | ||
| } | ||
|
|
||
| fn consts( | ||
| &mut self, | ||
| a: &'tcx ty::Const<'tcx>, | ||
| b: &'tcx ty::Const<'tcx>, | ||
| ) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> { | ||
| ty::relate::super_relate_consts(self, a, b) | ||
| } | ||
|
|
||
| fn binders<T>( | ||
| &mut self, | ||
| a: &ty::Binder<T>, | ||
| b: &ty::Binder<T>, | ||
| ) -> RelateResult<'tcx, ty::Binder<T>> | ||
| where | ||
| T: Relate<'tcx>, | ||
| { | ||
| self.relate(a.skip_binder(), b.skip_binder())?; | ||
| Ok(a.clone()) | ||
| } | ||
| } | ||
|
|
||
| // Instantiate and run relation. | ||
| let mut relator: LifetimeIgnoreRelation<'tcx> = LifetimeIgnoreRelation { tcx: tcx, param_env }; | ||
| relator.relate(&src, &dest).is_ok() | ||
| } | ||
|
|
||
| struct TypeChecker<'a, 'tcx> { | ||
| when: &'a str, | ||
| source: MirSource<'tcx>, | ||
|
|
@@ -81,6 +172,31 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |
| self.fail(location, format!("encountered jump to invalid basic block {:?}", bb)) | ||
| } | ||
| } | ||
|
|
||
| /// Check if src can be assigned into dest. | ||
| /// This is not precise, it will accept some incorrect assignments. | ||
| fn mir_assign_valid_types(&self, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { | ||
| if src == dest { | ||
| // Equal types, all is good. | ||
| return true; | ||
| } | ||
| // Normalize projections and things like that. | ||
| // FIXME: We need to reveal_all, as some optimizations change types in ways | ||
| // that require unfolding opaque types. | ||
| let param_env = self.param_env.with_reveal_all(); | ||
| let src = self.tcx.normalize_erasing_regions(param_env, src); | ||
| let dest = self.tcx.normalize_erasing_regions(param_env, dest); | ||
| // It's worth checking equality again. | ||
| if src == dest { | ||
| return true; | ||
| } | ||
|
|
||
| // Type-changing assignments can happen when subtyping is used. While | ||
| // all normal lifetimes are erased, higher-ranked types with their | ||
| // late-bound lifetimes are still around and can lead to type | ||
| // differences. So we compare ignoring lifetimes. | ||
| equal_up_to_regions(self.tcx, param_env, src, dest) | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | ||
|
|
@@ -99,20 +215,37 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | |
| } | ||
|
|
||
| fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { | ||
| match &statement.kind { | ||
| StatementKind::Assign(box (dest, rvalue)) => { | ||
|
There was a problem hiding this comment. As a follow up we should also check function calls and There was a problem hiding this comment.
There was a problem hiding this comment. Let's collect these ideas in #73832 |
||
| // LHS and RHS of the assignment must have the same type. | ||
| let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; | ||
| let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); | ||
| if !self.mir_assign_valid_types(right_ty, left_ty) { | ||
| self.fail( | ||
| location, | ||
| format!( | ||
| "encountered `Assign` statement with incompatible types:\n\ | ||
| left-hand side has type: {}\n\ | ||
| right-hand side has type: {}", | ||
| left_ty, right_ty, | ||
| ), | ||
| ); | ||
| } | ||
| // The sides of an assignment must not alias. Currently this just checks whether the places | ||
| // are identical. | ||
| match rvalue { | ||
| Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { | ||
| if dest == src { | ||
| self.fail( | ||
| location, | ||
| "encountered `Assign` statement with overlapping memory", | ||
| ); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is kind of surprising for me, afaik any kind of dispatch based on lifetimes is unsound and should not be possible.
i.e. one either has to implement something only for
'staticor for all lifetimes. Am I missing something here?edit: even if an impl is bound on an outlives relation (
'a, 'b: 'a) it should still be uniqueDo you have a somewhat concrete example where this can be relevant?
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.
Uh you'll need to ask @eddyb for that.^^
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.
FWIW, when I check
equal_up_to_regionsfirst and then just debug-assert that the layouts are equal, all tests pass. There might be a latent ICE there but I am not sure.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 personally would love to do this by default, but it's kind of bad to cause ICE just to satisfy my own curiosity 😆
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 think it is important that we understand our compiler.^^ So I changed this now to ICE on layout mismatch. If we get a bug report, at least we have a testcase. ;)
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.
The dispatch is on things like
for<'a> fn(&'a T)which are distinct fromfn(&'x T)(where'xgets erased).Besides associated types,
type_nameandTypeIdcan observe it. This goes as deep as symbol names: thev0mangling has to encode bound lifetimes in types because there's nothing else telling them apart.@nikomatsakis may have a better explanation as to why things are the way they are.
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.
@eddyb what would be a concrete example of this? If the current code here can ICE, would be nice to have a testcase.
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 have something that behaves differently for
for<'a> fn(&'a ())andfn(&'a ()). It does give acoherence_leak_checkback-compat warning though: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.
The reason for this is somewhat historical, and indeed I do have some hopes of changing things here, although I think I've realized that we can't make all the changes I had hoped for. This example I think will eventually be accepted, despite the future compatibility warning, since we really need to accept that to accept existing patterns in the wild. But the fact that these impls are accepted therefore implis the importance of binding for e.g. symbol names.