Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mir tycheck aggregate rvalues #13

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions src/librustc_mir/transform/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,7 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
terr
);
}
self.check_rvalue(mir, rv, location);
}
StatementKind::SetDiscriminant {
ref lvalue,
Expand Down Expand Up @@ -1008,6 +1009,91 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
}
}

fn aggregate_field_ty(&mut self, ak: &Box<AggregateKind<'tcx>>, field: usize, location: Location)
-> Result<Ty<'tcx>, FieldAccessError>
{
let tcx = self.tcx();

let (variant, substs) = match **ak {
AggregateKind::Adt(def, variant, substs, _) => { // handle unions?
(&def.variants[variant], substs)
},
AggregateKind::Closure(def_id, substs) => {
return match substs.upvar_tys(def_id, tcx).nth(field) {
Some(ty) => Ok(ty),
None => Err(FieldAccessError::OutOfRange {
field_count: substs.upvar_tys(def_id, tcx).count()
}),
}
},
AggregateKind::Generator(def_id, substs, _) => {
if let Some(ty) = substs.upvar_tys(def_id, tcx).nth(field) {
return Ok(ty);
}

return match substs.field_tys(def_id, tcx).nth(field) {
Some(ty) => Ok(ty),
None => Err(FieldAccessError::OutOfRange {
field_count: substs.field_tys(def_id, tcx).count() + 1
}),
}
},
AggregateKind::Array(ty) => {
return Ok(ty);
},
AggregateKind::Tuple => {
unreachable!("This should have been covered in check_rvalues");
},
};

if let Some(field) = variant.fields.get(field) {
Ok(self.normalize(&field.ty(tcx, substs), location))
} else {
Err(FieldAccessError::OutOfRange { field_count: variant.fields.len() })
}
}

#[allow(dead_code)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why dead_code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was writing check_rvalue before I was using it and was trying to check the types. This is cleaned up in the latest commit.

fn check_rvalue(&mut self, mir: &Mir<'tcx>, rv: &Rvalue<'tcx>, location: Location) {
let tcx = self.tcx();
match rv {
Rvalue::Aggregate(ref ak, ref ops) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ref ought to be the default; we're matching on a reference

match **ak {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you no longer need match **ak, thanks to #[feature(match_default_bindings)]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit I still need to because this is a &Box rather than &&.

// tuple rvalue field type is always the type of the op. Nothing to check here.
AggregateKind::Tuple => { },
_ => {
for (i, op) in ops.iter().enumerate() {
let field_ty = if let Ok(field_ty) = self.aggregate_field_ty(ak, i, location) {
field_ty
} else {
// TODO(nashenas88) log span_mirbug terr??
continue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think span_mirbug is appropriate here.

};
let op_ty = match op {
Operand::Consume(lv) => lv.ty(mir, tcx).to_ty(tcx),
Operand::Constant(c) => c.ty,
};
if let Err(_terr) = self.sub_types(op_ty, field_ty, location.at_successor_within_block()) {
// TODO(nashenas88) log span_mirbug terr??
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, span_mirbug seems good.

}
}
},
}
},
// FIXME: These other cases have to be implemented in future PRs
Rvalue::Use(..) |
Rvalue::Repeat(..) |
Rvalue::Ref(..) |
Rvalue::Len(..) |
Rvalue::Cast(..) |
Rvalue::BinaryOp(..) |
Rvalue::CheckedBinaryOp(..) |
Rvalue::UnaryOp(..) |
Rvalue::Discriminant(..) |
Rvalue::NullaryOp(..) => { }
}
}

fn typeck_mir(&mut self, mir: &Mir<'tcx>) {
self.last_span = mir.span;
debug!("run_on_mir: {:?}", mir.span);
Expand Down
29 changes: 29 additions & 0 deletions src/test/compile-fail/aggregate-rvalues-typeck.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
//revisions: ast mir
//[mir] compile-flags: -Z emit-end-regions -Z borrowck-mir -Z nll

#![allow(unused_assignments)]

struct Wrap<'a> { w: &'a mut u32 }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this test to compile-fail/nll/reference-carried-through-struct-field.rs. Not sure what the best naming scheme is here, but at least in the nll directory would be good.


fn foo() {
let mut x = 22;
let wrapper = Wrap { w: &mut x };
//~^ ERROR cannot assign to `x` because it is borrowed (Mir) [E0506]
//~^^ ERROR cannot use `x` because it was mutably borrowed (Mir) [E0503]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh ,do we get errors here too? These seem bogus...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are actually ignored by the test. I accidentally left them in from the run before I added in revisions. It seems with revisions only comments beginning with a revision tag are looked at. These are gone in the latest commit.

x += 1; //[ast]~ ERROR cannot assign to `x` because it is borrowed [E0506]
//[mir]~^ ERROR cannot assign to `x` because it is borrowed (Ast) [E0506]
//[mir]~^^ ERROR cannot assign to `x` because it is borrowed (Mir) [E0506]
//[mir]~^^^ ERROR cannot use `x` because it was mutably borrowed (Mir) [E0503]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these seem correct.

*wrapper.w += 1;
}

fn main() { }