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

Trait-associated const fixes #25091

Merged
merged 4 commits into from
May 26, 2015
Merged
Show file tree
Hide file tree
Changes from 3 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
48 changes: 35 additions & 13 deletions src/librustc/middle/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,10 @@ pub fn lookup_const_by_id<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
Some(ref_id) => {
let trait_id = ty::trait_of_item(tcx, def_id)
.unwrap();
let substs = ty::node_id_item_substs(tcx, ref_id)
.substs;
resolve_trait_associated_const(tcx, ti, trait_id,
ref_id)
substs)
}
// Technically, without knowing anything about the
// expression that generates the obligation, we could
Expand Down Expand Up @@ -172,8 +174,10 @@ pub fn lookup_const_by_id<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
// a trait-associated const if the caller gives us
// the expression that refers to it.
Some(ref_id) => {
let substs = ty::node_id_item_substs(tcx, ref_id)
.substs;
resolve_trait_associated_const(tcx, ti, trait_id,
ref_id).map(|e| e.id)
substs).map(|e| e.id)
}
None => None
}
Expand Down Expand Up @@ -633,9 +637,23 @@ pub_fn_checked_op!{ const_uint_checked_shr_via_int(a: u64, b: i64,.. UintTy) {
uint_shift_body overflowing_shr const_uint ShiftRightWithOverflow
}}

// After type checking, `eval_const_expr_partial` should always suffice. The
// reason for providing `eval_const_expr_with_substs` is to allow
// trait-associated consts to be evaluated *during* type checking, when the
// substs for each expression have not been written into `tcx` yet.
pub fn eval_const_expr_partial<'tcx>(tcx: &ty::ctxt<'tcx>,
e: &Expr,
ty_hint: Option<Ty<'tcx>>) -> EvalResult {
eval_const_expr_with_substs(tcx, e, ty_hint, |id| {
ty::node_id_item_substs(tcx, id).substs
})
}

pub fn eval_const_expr_with_substs<'tcx, S>(tcx: &ty::ctxt<'tcx>,
e: &Expr,
ty_hint: Option<Ty<'tcx>>,
get_substs: S) -> EvalResult
where S: Fn(ast::NodeId) -> subst::Substs<'tcx> {
fn fromb(b: bool) -> const_val { const_int(b as i64) }

let ety = ty_hint.or_else(|| ty::expr_ty_opt(tcx, e));
Expand Down Expand Up @@ -826,8 +844,11 @@ pub fn eval_const_expr_partial<'tcx>(tcx: &ty::ctxt<'tcx>,
def::FromTrait(trait_id) => match tcx.map.find(def_id.node) {
Some(ast_map::NodeTraitItem(ti)) => match ti.node {
ast::ConstTraitItem(ref ty, _) => {
(resolve_trait_associated_const(tcx, ti,
trait_id, e.id),
let substs = get_substs(e.id);
(resolve_trait_associated_const(tcx,
ti,
trait_id,
substs),
Some(&**ty))
}
_ => (None, None)
Expand Down Expand Up @@ -926,10 +947,9 @@ pub fn eval_const_expr_partial<'tcx>(tcx: &ty::ctxt<'tcx>,
fn resolve_trait_associated_const<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
ti: &'tcx ast::TraitItem,
trait_id: ast::DefId,
ref_id: ast::NodeId)
rcvr_substs: subst::Substs<'tcx>)
-> Option<&'tcx Expr>
{
let rcvr_substs = ty::node_id_item_substs(tcx, ref_id).substs;
let subst::SeparateVecsPerParamSpace {
types: rcvr_type,
selfs: rcvr_self,
Expand Down Expand Up @@ -1081,19 +1101,21 @@ pub fn compare_const_vals(a: &const_val, b: &const_val) -> Option<Ordering> {
})
}

pub fn compare_lit_exprs<'tcx>(tcx: &ty::ctxt<'tcx>,
a: &Expr,
b: &Expr,
ty_hint: Option<Ty<'tcx>>)
-> Option<Ordering> {
let a = match eval_const_expr_partial(tcx, a, ty_hint) {
pub fn compare_lit_exprs<'tcx, S>(tcx: &ty::ctxt<'tcx>,
a: &Expr,
b: &Expr,
ty_hint: Option<Ty<'tcx>>,
get_substs: S) -> Option<Ordering>
where S: Fn(ast::NodeId) -> subst::Substs<'tcx> {
let a = match eval_const_expr_with_substs(tcx, a, ty_hint,
|id| {get_substs(id)}) {
Ok(a) => a,
Err(e) => {
tcx.sess.span_err(a.span, &e.description());
return None;
}
};
let b = match eval_const_expr_partial(tcx, b, ty_hint) {
let b = match eval_const_expr_with_substs(tcx, b, ty_hint, get_substs) {
Ok(b) => b,
Err(e) => {
tcx.sess.span_err(b.span, &e.description());
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_trans/trans/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ struct ConstantExpr<'a>(&'a ast::Expr);

impl<'a> ConstantExpr<'a> {
fn eq(self, other: ConstantExpr<'a>, tcx: &ty::ctxt) -> bool {
match const_eval::compare_lit_exprs(tcx, self.0, other.0, None) {
match const_eval::compare_lit_exprs(tcx, self.0, other.0, None,
|id| {ty::node_id_item_substs(tcx, id).substs}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the usual thing here is not to pass a closure, but rather a Typer -- but I'm not sure off-hand if we have a method getting the substs of an id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the other thing is that const_eval has to be working already in collect, and I'm trying to get resolution of trait-associated constants working there. We don't have ClosureTyper and ParameterEnvironment in play at that point, and it would be kind of funky to use them that early.

Some(result) => result == Ordering::Equal,
None => panic!("compare_list_exprs: type mismatch"),
}
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_typeck/check/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
lhs_eq_rhs && (ty::type_is_numeric(lhs_ty) || ty::type_is_char(lhs_ty));

if numeric_or_char {
match const_eval::compare_lit_exprs(tcx, &**begin, &**end, Some(lhs_ty)) {
match const_eval::compare_lit_exprs(tcx, &**begin, &**end, Some(lhs_ty),
|id| {fcx.item_substs()[&id].substs
.clone()}) {
Some(Ordering::Less) |
Some(Ordering::Equal) => {}
Some(Ordering::Greater) => {
Expand Down
31 changes: 31 additions & 0 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3763,8 +3763,36 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>,
&'a [ast::PathSegment],
def::Def)>
{

// Associated constants can't depend on generic types.
fn have_disallowed_generic_consts<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
def: def::Def,
ty: Ty<'tcx>,
span: Span,
node_id: ast::NodeId) -> bool {
match def {
def::DefAssociatedConst(..) => {
if ty::type_has_params(ty) || ty::type_has_self(ty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably need to take lifetime parameters into account as well -- I don't know if there's an easy predicate to test for that, though. It seems like it'd be nice have a single flag for "NEEDS_SUBST" that applies to any type which references a ty_param or ReEarlyBound.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Note that lifetime parameters on the trait will be early bound)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure that lifetime parameters are relevant? The reason that type parameters are relevant is that they can influence the constant value that you end up with. I don't see how this could be true for lifetime parameters. E.g. I think that if <&'a Foo>::CONST can be resolved at all, it should resolve to the same thing as <&'b Foo>::CONST and <&'static Foo>::CONST?

Hmm, except if you have a trait impl that applies to <&'a Foo>::CONST, and an inherent impl that applies only <&'static Foo>::CONST, which is sort of like a specialization for 'static? Is that what you mean? Never mind, I don't think that makes any sense since we are talking about issues with generic code where inherent impls are irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

@quantheory

Are you sure that lifetime parameters are relevant?

So, I think that lifetime parameters are as relevant as type parameters, but thinking on this a bit more I'm actually unclear on why the restriction on type parameters is needed. Am I missing something obvious? Perhaps you can spell out the scenario for me that you are concerned about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary restriction to prevent ICEs like #25046, until something like rust-lang/rfcs#1062 is implemented. Right now we expect to be able to evaluate constants in typeck, but if the constant's value depends on a type parameter, it can't be evaluated until trans. To deal with generic constants, we can use something like a projection, but this PR is much easier to implement in the meantime.

The only reason that a type parameter can affect the value of a const seems to be because it can affect which const we actually resolve to. This doesn't seem to be true of lifetime parameters, so I don't think that we need to worry about them. (And this makes some sense, because constants are... constant, more like statics or macros than something that's restricted to a particular region.) In any case, I haven't been able to construct an equivalent of #25046 that crashes due to a rogue lifetime parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see, I was a bit confused. As a temporary bypass for an ICE, this change (probably) makes sense. I thought at first you were imposing a more permanent restriction against associated constants whose types depending on type parameters -- and this didn't seem to make any sense to me, as I don't see any fundamental problem there.

We do need to get our story straight about constants that cannot be resolved until trans, clearly -- seems to me that "constant equality" ought to be handled in a similar way to "type equality" in any case (and hence constant evaluation at type-check time is basically a form of normalization). But I have to finish a write-up talking about normalization and how we should consider a (mildly) different approach there too, so I guess it would apply equally.

Anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a tracking issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. I'd like to get there, but have been tangled up in #22519 and related issues.

fcx.sess().span_err(span,
"Associated consts cannot depend \
on type parameters or Self.");
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a diagnostic (and associated code) use the span_err! macro?

fcx.write_error(node_id);
return true;
}
}
_ => {}
}
false
}

// If fully resolved already, we don't have to do anything.
if path_res.depth == 0 {
if let Some(ty) = opt_self_ty {
if have_disallowed_generic_consts(fcx, path_res.full_def(), ty,
span, node_id) {
return None;
}
}
Some((opt_self_ty, &path.segments, path_res.base_def))
} else {
let mut def = path_res.base_def;
Expand All @@ -3780,6 +3808,9 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>,
let item_name = item_segment.identifier.name;
match method::resolve_ufcs(fcx, span, item_name, ty, node_id) {
Ok((def, lp)) => {
if have_disallowed_generic_consts(fcx, def, ty, span, node_id) {
return None;
}
// Write back the new resolution.
fcx.ccx.tcx.def_map.borrow_mut()
.insert(node_id, def::PathResolution {
Expand Down
26 changes: 26 additions & 0 deletions src/test/compile-fail/associated-const-type-parameters.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2015 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.

#![feature(associated_consts)]

pub trait Foo {
const MIN: i32;

fn get_min() -> i32 {
Self::MIN //~ Associated consts cannot depend on type parameters or Self.
}
}

fn get_min<T: Foo>() -> i32 {
T::MIN; //~ Associated consts cannot depend on type parameters or Self.
<T as Foo>::MIN //~ Associated consts cannot depend on type parameters or Self.
}

fn main() {}
35 changes: 35 additions & 0 deletions src/test/run-pass/associated-const-range-match-patterns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2015 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.

#![feature(associated_consts)]

struct Foo;

trait HasNum {
const NUM: isize;
}
impl HasNum for Foo {
const NUM: isize = 1;
}

fn main() {
assert!(match 2 {
Foo::NUM ... 3 => true,
_ => false,
});
assert!(match 0 {
-1 ... <Foo as HasNum>::NUM => true,
_ => false,
});
assert!(match 1 {
<Foo as HasNum>::NUM ... <Foo>::NUM => true,
_ => false,
});
}