Semantic check of mut restrictions#159906
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
mut restrictions
| if let hir::RestrictionKind::Restricted(path) = impl_restriction.kind { | ||
| ty::trait_def::ImplRestrictionKind::Restricted(path.res, impl_restriction.span) | ||
| ty::RestrictionKind::Restricted(path.res, impl_restriction.span) | ||
| } else { | ||
| ty::trait_def::ImplRestrictionKind::Unrestricted | ||
| ty::RestrictionKind::Unrestricted | ||
| }, |
There was a problem hiding this comment.
Could you also make this else-if an match instead, as to be exhaustive.
| // MIR-level lints. | ||
| &Lint(check_inline::CheckForceInline), | ||
| &Lint(check_call_recursion::CheckCallRecursion), | ||
| &Lint(check_packed_ref::CheckPackedRef), | ||
| &Lint(check_const_item_mutation::CheckConstItemMutation), | ||
| &Lint(check_mut_restriction::CheckMutRestriction), | ||
| &Lint(function_item_references::FunctionItemReferences), |
There was a problem hiding this comment.
(Not for this PR) Most of those so called "MIR lints" actually emit hard errors of an lint. We should find another name as it's quite confusing to see a "lint" emit a hard errors.
| struct MutRestrictionChecker<'a, 'tcx> { | ||
| body: &'a Body<'tcx>, | ||
| tcx: TyCtxt<'tcx>, | ||
| span: Span, |
There was a problem hiding this comment.
| span: Span, | |
| mutating_span: Span, |
| let body_did = self.body.source.instance.def_id(); | ||
|
|
||
| for (place_base, elem) in place.iter_projections() { | ||
| let ProjectionElem::Field(field_idx, _field_ty) = elem else { |
There was a problem hiding this comment.
Could you add some comment about why we are only checking for ProjectionElem::Field, I assume that for an index operation foo.bar[index] we always have a field operation.
| }; | ||
|
|
||
| let base_ty = place_base.ty(self.body, self.tcx); | ||
| let ty::Adt(adt_def, _args) = *base_ty.ty.kind() else { |
There was a problem hiding this comment.
Same, please add a comment here for why only ty::Adt and why we ignore args.
| //@ revisions: e2015 e2018 | ||
| //@ [e2015] edition: 2015 | ||
| //@ [e2018] edition: 2018.. |
There was a problem hiding this comment.
Same here, no need for both revisions, let's stick 2018 an onward.
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo` | ||
| bar.2 = 1; // ok | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should also check for inline assembly mutating use.
// for x86-64
asm!(
"add {0}, 5",
"mov {1}, 42",
inout(reg) state.inout_field, // mutable inout_field
out(reg) state.out_field, // mutable out_field
);| foo::bar::FooE::Var { | ||
| alpha, //[e2015]~ ERROR field cannot be mutated outside `foo::bar` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo::bar` | ||
| beta, //[e2015]~ ERROR field cannot be mutated outside `foo` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo` | ||
| gamma, // ok | ||
| } => { |
There was a problem hiding this comment.
What happens if I someone does alpha: _? Do we report an error?
Should probably also be a test.
| foo::bar::FooE::Tup( | ||
| alpha, //[e2015]~ ERROR field cannot be mutated outside `foo::bar` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo::bar` | ||
| beta, //[e2015]~ ERROR field cannot be mutated outside `foo` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo` | ||
| gamma, // ok | ||
| ) => { |
There was a problem hiding this comment.
We should also test what happens with an immutable borrowing:
&foo::bar::FooE::Tup(| fn change_fooe(foo: &mut foo::bar::FooE) { | ||
| match foo { | ||
| foo::bar::FooE::Default => {} | ||
| foo::bar::FooE::Var { | ||
| alpha, //[e2015]~ ERROR field cannot be mutated outside `foo::bar` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo::bar` | ||
| beta, //[e2015]~ ERROR field cannot be mutated outside `foo` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo` | ||
| gamma, // ok | ||
| } => { | ||
| *alpha = 1; | ||
| *beta = 1; | ||
| *gamma = 1; | ||
| } | ||
| foo::bar::FooE::Tup( | ||
| alpha, //[e2015]~ ERROR field cannot be mutated outside `foo::bar` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo::bar` | ||
| beta, //[e2015]~ ERROR field cannot be mutated outside `foo` | ||
| //[e2018]~^ ERROR field cannot be mutated outside `crate::foo` | ||
| gamma, // ok | ||
| ) => { | ||
| *alpha = 1; | ||
| *beta = 1; | ||
| *gamma = 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@Nadrieril, can you think of another match related tests we should be testing regarding mut-restrictions?
|
cc @cjgillot for the MIR related part since you know more about it than me |
This PR implements semantic checks for
mutrestrictions proposed in RFC 3323, as part of the related GSoC project.It lowers field restriction information from HIR into
rustc_middle::ty::FieldDefand adds a MIR-level lint that rejects restricted field mutations and struct expressions. As parsing (#156824) and path resolution (#159125) have already been implemented, this PR provides a working implementation ofmutrestrictions.Tracking issue: #105077
r? @Urgau
cc @jhpratt