Skip to content

Support generic associated consts #30446

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

Merged
merged 1 commit into from
Jan 15, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 2 additions & 5 deletions src/librustc/middle/check_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,13 +657,10 @@ fn check_expr<'a, 'tcx>(v: &mut CheckCrateVisitor<'a, 'tcx>,
Some(def::DefConst(did)) |
Some(def::DefAssociatedConst(did)) => {
if let Some(expr) = const_eval::lookup_const_by_id(v.tcx, did,
Some(e.id)) {
Some(e.id),
None) {
let inner = v.global_expr(Mode::Const, expr);
v.add_qualif(inner);
} else {
v.tcx.sess.span_bug(e.span,
"DefConst or DefAssociatedConst \
doesn't point to a constant");
}
}
Some(def::DefLocal(..)) if v.mode == Mode::ConstFn => {
Expand Down
3 changes: 2 additions & 1 deletion src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ impl<'a, 'tcx> Folder for StaticInliner<'a, 'tcx> {
let def = self.tcx.def_map.borrow().get(&pat.id).map(|d| d.full_def());
match def {
Some(DefAssociatedConst(did)) |
Some(DefConst(did)) => match lookup_const_by_id(self.tcx, did, Some(pat.id)) {
Some(DefConst(did)) => match lookup_const_by_id(self.tcx, did,
Some(pat.id), None) {
Some(const_expr) => {
const_expr_to_pat(self.tcx, const_expr, pat.span).map(|new_pat| {

Expand Down
41 changes: 26 additions & 15 deletions src/librustc/middle/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use front::map as ast_map;
use front::map::blocks::FnLikeNode;
use middle::cstore::{self, CrateStore, InlinedItem};
use middle::{def, infer, subst, traits};
use middle::subst::Subst;
use middle::def_id::DefId;
use middle::pat_util::def_to_path;
use middle::ty::{self, Ty};
Expand Down Expand Up @@ -48,7 +49,7 @@ fn lookup_const<'a>(tcx: &'a ty::ctxt, e: &Expr) -> Option<&'a Expr> {
match opt_def {
Some(def::DefConst(def_id)) |
Some(def::DefAssociatedConst(def_id)) => {
lookup_const_by_id(tcx, def_id, Some(e.id))
lookup_const_by_id(tcx, def_id, Some(e.id), None)
}
Some(def::DefVariant(enum_def, variant_def, _)) => {
lookup_variant_by_id(tcx, enum_def, variant_def)
Expand Down Expand Up @@ -88,9 +89,17 @@ fn lookup_variant_by_id<'a>(tcx: &'a ty::ctxt,
}
}

/// * `def_id` is the id of the constant.
/// * `maybe_ref_id` is the id of the expr referencing the constant.
/// * `param_substs` is the monomorphization substitution for the expression.
///
/// `maybe_ref_id` and `param_substs` are optional and are used for
/// finding substitutions in associated constants. This generally
/// happens in late/trans const evaluation.
pub fn lookup_const_by_id<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
def_id: DefId,
maybe_ref_id: Option<ast::NodeId>)
maybe_ref_id: Option<ast::NodeId>,
param_substs: Option<&'tcx subst::Substs<'tcx>>)
Copy link
Contributor

Choose a reason for hiding this comment

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

This merits a comment, I think, as to the purpose of each of these parameters. Something like:

  • def_id is the id of the constant
  • maybe_ref_id is the id of the expression referencing the constant, if available
  • param_substs is the monomorphization substitution for the expression, if available

The latter two parameters are used to extract the substitutions for the constant, in the case of associated constants.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside -- this all feels very hokey to me, I'd rather we just passed in the final set of substitutions, rather than passing in an expr-id and monomorphization substitution and then figuring out the substitutions. But that feels like a separate PR.

UPDATE: To be clear: I wasn't calling your diff hokey. This fn is just getting increasingly complex. In general, it just seems better to me overall if we can pass in "this definition with these substitutions" rather than "this definition referenced from this context" (the latter being how the fn is currently setup).

-> Option<&'tcx Expr> {
if let Some(node_id) = tcx.map.as_local_node_id(def_id) {
match tcx.map.find(node_id) {
Expand All @@ -111,8 +120,11 @@ pub fn lookup_const_by_id<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
Some(ref_id) => {
let trait_id = tcx.trait_of_item(def_id)
.unwrap();
let substs = tcx.node_id_item_substs(ref_id)
.substs;
let mut substs = tcx.node_id_item_substs(ref_id)
.substs;
if let Some(param_substs) = param_substs {
substs = substs.subst(tcx, param_substs);
}
resolve_trait_associated_const(tcx, ti, trait_id,
substs)
}
Expand Down Expand Up @@ -158,8 +170,11 @@ 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 = tcx.node_id_item_substs(ref_id)
.substs;
let mut substs = tcx.node_id_item_substs(ref_id)
.substs;
if let Some(param_substs) = param_substs {
substs = substs.subst(tcx, param_substs);
}
resolve_trait_associated_const(tcx, ti, trait_id,
substs).map(|e| e.id)
}
Expand Down Expand Up @@ -1013,7 +1028,7 @@ pub fn eval_const_expr_partial<'tcx>(tcx: &ty::ctxt<'tcx>,
_ => (None, None)
}
} else {
(lookup_const_by_id(tcx, def_id, Some(e.id)), None)
(lookup_const_by_id(tcx, def_id, Some(e.id), None), None)
}
}
Some(def::DefAssociatedConst(def_id)) => {
Expand Down Expand Up @@ -1048,7 +1063,7 @@ pub fn eval_const_expr_partial<'tcx>(tcx: &ty::ctxt<'tcx>,
},
}
} else {
(lookup_const_by_id(tcx, def_id, Some(e.id)), None)
(lookup_const_by_id(tcx, def_id, Some(e.id), None), None)
}
}
Some(def::DefVariant(enum_def, variant_def, _)) => {
Expand Down Expand Up @@ -1260,20 +1275,16 @@ fn resolve_trait_associated_const<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
Ok(None) => {
return None
}
Err(e) => {
tcx.sess.span_bug(ti.span,
&format!("Encountered error `{:?}` when trying \
to select an implementation for \
constant trait item reference.",
e))
Err(_) => {
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelwu why did you change this to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC this was to avoid hitting an error in early const eval. fn select() returns Err(_) if there's a substitution that can't be found. Maybe it should return Ok(None)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Yes, I suspect calling bug was wrong. I don't have a great grasp on where an error on the user's part will fall out here, but I guess I would expect to see it come out during typeck. I should check if there are existing test cases.

}
};

match selection {
traits::VtableImpl(ref impl_data) => {
match tcx.associated_consts(impl_data.impl_def_id)
.iter().find(|ic| ic.name == ti.name) {
Some(ic) => lookup_const_by_id(tcx, ic.def_id, None),
Some(ic) => lookup_const_by_id(tcx, ic.def_id, None, None),
None => match ti.node {
hir::ConstTraitItem(_, Some(ref expr)) => Some(&*expr),
_ => None,
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/hair/cx/pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ impl<'patcx, 'cx, 'tcx> PatCx<'patcx, 'cx, 'tcx> {
let def = self.cx.tcx.def_map.borrow().get(&pat.id).unwrap().full_def();
match def {
def::DefConst(def_id) | def::DefAssociatedConst(def_id) =>
match const_eval::lookup_const_by_id(self.cx.tcx, def_id, Some(pat.id)) {
match const_eval::lookup_const_by_id(self.cx.tcx, def_id,
Some(pat.id), None) {
Some(const_expr) => {
let pat = const_eval::const_expr_to_pat(self.cx.tcx, const_expr,
pat.span);
Expand Down
19 changes: 11 additions & 8 deletions src/librustc_trans/trans/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ fn const_fn_call<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,

pub fn get_const_expr<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
def_id: DefId,
ref_expr: &hir::Expr)
ref_expr: &hir::Expr,
param_substs: &'tcx Substs<'tcx>)
-> &'tcx hir::Expr {
let def_id = inline::maybe_instantiate_inline(ccx, def_id);

Expand All @@ -226,7 +227,7 @@ pub fn get_const_expr<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
"cross crate constant could not be inlined");
}

match const_eval::lookup_const_by_id(ccx.tcx(), def_id, Some(ref_expr.id)) {
match const_eval::lookup_const_by_id(ccx.tcx(), def_id, Some(ref_expr.id), Some(param_substs)) {
Some(ref expr) => expr,
None => {
ccx.sess().span_bug(ref_expr.span, "constant item not found")
Expand Down Expand Up @@ -264,10 +265,12 @@ pub enum TrueConst {

use self::ConstEvalFailure::*;

fn get_const_val(ccx: &CrateContext,
def_id: DefId,
ref_expr: &hir::Expr) -> Result<ValueRef, ConstEvalFailure> {
let expr = get_const_expr(ccx, def_id, ref_expr);
fn get_const_val<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
def_id: DefId,
ref_expr: &hir::Expr,
param_substs: &'tcx Substs<'tcx>)
-> Result<ValueRef, ConstEvalFailure> {
let expr = get_const_expr(ccx, def_id, ref_expr, param_substs);
let empty_substs = ccx.tcx().mk_substs(Substs::trans_empty());
match get_const_expr_as_global(ccx, expr, check_const::ConstQualif::empty(),
empty_substs, TrueConst::Yes) {
Expand Down Expand Up @@ -297,7 +300,7 @@ pub fn get_const_expr_as_global<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
if !ccx.tcx().tables.borrow().adjustments.contains_key(&expr.id) {
debug!("get_const_expr_as_global ({:?}): found const {:?}",
expr.id, def_id);
return get_const_val(ccx, def_id, expr);
return get_const_val(ccx, def_id, expr, param_substs);
}
},
_ => {},
Expand Down Expand Up @@ -888,7 +891,7 @@ fn const_expr_unadjusted<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
expr::trans_def_fn_unadjusted(cx, e, def, param_substs).val
}
def::DefConst(def_id) | def::DefAssociatedConst(def_id) => {
const_deref_ptr(cx, try!(get_const_val(cx, def_id, e)))
const_deref_ptr(cx, try!(get_const_val(cx, def_id, e, param_substs)))
}
def::DefVariant(enum_did, variant_did, _) => {
let vinfo = cx.tcx().lookup_adt_def(enum_did).variant_with_id(variant_did);
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_trans/trans/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ pub fn trans_into<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
hir::ExprPath(..) => {
match bcx.def(expr.id) {
def::DefConst(did) => {
let const_expr = consts::get_const_expr(bcx.ccx(), did, expr);
let empty_substs = bcx.tcx().mk_substs(Substs::trans_empty());
let const_expr = consts::get_const_expr(bcx.ccx(), did, expr,
empty_substs);
// Temporarily get cleanup scopes out of the way,
// as they require sub-expressions to be contained
// inside the current AST scope.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/mir/did.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
},
ItemKind::Constant => {
let did = inline::maybe_instantiate_inline(bcx.ccx(), did);
let expr = const_eval::lookup_const_by_id(bcx.tcx(), did, None)
let expr = const_eval::lookup_const_by_id(bcx.tcx(), did, None, Some(substs))
.expect("def was const, but lookup_const_by_id failed");
// FIXME: this is falling back to translating from HIR. This is not easy to fix,
// because we would have somehow adapt const_eval to work on MIR rather than HIR.
Expand Down
30 changes: 0 additions & 30 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3785,35 +3785,8 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>,
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.has_param_types() || ty.has_self_ty() {
span_err!(fcx.sess(), span, E0329,
"Associated consts cannot depend \
on type parameters or Self.");
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 @@ -3829,9 +3802,6 @@ 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
4 changes: 2 additions & 2 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ pub fn build_impl(cx: &DocContext,
let did = assoc_const.def_id;
let type_scheme = tcx.lookup_item_type(did);
let default = if assoc_const.has_value {
Some(const_eval::lookup_const_by_id(tcx, did, None)
Some(const_eval::lookup_const_by_id(tcx, did, None, None)
.unwrap().span.to_src(cx))
} else {
None
Expand Down Expand Up @@ -479,7 +479,7 @@ fn build_const(cx: &DocContext, tcx: &ty::ctxt,
use rustc::middle::const_eval;
use rustc_front::print::pprust;

let expr = const_eval::lookup_const_by_id(tcx, did, None).unwrap_or_else(|| {
let expr = const_eval::lookup_const_by_id(tcx, did, None, None).unwrap_or_else(|| {
panic!("expected lookup_const_by_id to succeed for {:?}", did);
});
debug!("converting constant expr {:?} to snippet", expr);
Expand Down
21 changes: 21 additions & 0 deletions src/test/compile-fail/associated-const-array-len.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2016 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)]

trait Foo {
const ID: usize;
}

const X: [i32; <i32 as Foo>::ID] = [0, 1, 2]; //~ ERROR E0250

fn main() {
assert_eq!(1, X);
}
22 changes: 22 additions & 0 deletions src/test/compile-fail/associated-const-no-item.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2016 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)]

trait Foo {
const ID: i32;
}

const X: i32 = <i32>::ID;
//~^ ERROR no associated item named `ID` found for type `i32`

fn main() {
assert_eq!(1, X);
}
38 changes: 38 additions & 0 deletions src/test/compile-fail/associated-const-type-parameter-arms.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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 enum EFoo { A, B, C, D }

pub trait Foo {
const X: EFoo;
}

struct Abc;
impl Foo for Abc {
const X: EFoo = EFoo::B;
}

struct Def;
impl Foo for Def {
const X: EFoo = EFoo::D;
}

pub fn test<A: Foo, B: Foo>(arg: EFoo) {
match arg {
A::X => println!("A::X"), //~ error: statics cannot be referenced in patterns [E0158]
B::X => println!("B::X"), //~ error: statics cannot be referenced in patterns [E0158]
_ => (),
}
}

fn main() {
}
32 changes: 32 additions & 0 deletions src/test/compile-fail/associated-const-type-parameter-arrays-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// 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 Y: usize;
}

struct Abc;
impl Foo for Abc {
const Y: usize = 8;
}

struct Def;
impl Foo for Def {
const Y: usize = 33;
}

pub fn test<A: Foo, B: Foo>() {
let _array = [4; <A as Foo>::Y]; //~ error: expected constant integer
}

fn main() {
}
Loading