Support generic associated consts #30446

michaelwu · 2015-12-17T20:15:31Z

This provides limited support for using associated consts on type parameters. It generally works on things that can be figured out at trans time. This doesn't work for array lengths or match arms. I have another patch to make it work in const expressions.

CC @eddyb @nikomatsakis

rust-highfive · 2015-12-17T20:15:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

michaelwu · 2015-12-17T20:28:06Z

I think @nikomatsakis wanted to review this - didn't realize rust-highfive automatically assigned a reviewer.

oli-obk · 2015-12-18T09:34:05Z

the corresponding RFC has not yet been accepted: rust-lang/rfcs#1062

michaelwu · 2015-12-18T17:46:07Z

FWIW, this does a different thing than what the RFC proposes. That RFC attempts to make certain tricky situations work like array lengths and match patterns work. This doesn't do any of that - it only supports the parts that are simple to handle.

nikomatsakis · 2015-12-18T18:05:20Z

r? @nikomatsakis

nikomatsakis · 2015-12-18T18:05:54Z

stole review since @michaelwu and I already looked over this at work week

nikomatsakis · 2016-01-04T17:32:13Z

src/librustc/middle/const_eval.rs

-                                        constant trait item reference.",
-                                       e))
+        Err(_) => {
+            return None


@michaelwu why did you change this to None?

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)?

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.

nikomatsakis · 2016-01-04T23:21:14Z

so yes this looks overall good to me. I'd love to hear from @oli-obk, since he's been doing so much with constants, but I think I'm prepared to r+. I'm just trying to think if there is anything being unintentionally stabilized here -- I don't think so.

(I also agree that this is a distinct issue from what the RFC is addressing; this seems to be just pushing the current support for assoc constants further within the limits of the current design.)

oli-obk · 2016-01-05T19:56:19Z

Since this only touches trans (const eval passes None for the param substs) it should be fine. But it might allow more than we want. Currently it should not allow referencing an assoc const from an assoc const. But if check_const doesn't catch it, trans might simply allow it. I'm not sure if there's a compile-fail test for it.

nikomatsakis · 2016-01-05T22:31:20Z

@oli-obk my biggest worry are things being allowed without feature gates -- but it's good to identify bugs or deviations from the expected behavior. Everything I looked at seemed like it would be within bounds though. Why do you say it should not allow referencing an assoc. const from another assoc. const?

oli-obk · 2016-01-06T12:25:41Z

Since the entire thing is behind a feature gate it is ok. But if we allow things in assoc constants that we don't allow in true const eval, it will be much harder getting them to work with array lengths in the future.

nikomatsakis · 2016-01-06T14:48:04Z

@oli-obk

But if we allow things in assoc constants that we don't allow in true const eval, it will be much harder getting them to work with array lengths in the future.

I'm confused by this. I think you are saying: "early const eval (before trans) cannot handle assoc constants. If we want to allow associated constants to be handled by early const eval, we should ensure that they do not reference other associated constants, because those are not handled by early const eval". But it seems like they would be handled by early const eval if... they were handled by early const eval?

But in any case I think that for assoc constants to be allowed in array lengths, we would have to move beyond the notion of early/late const eval, which is (imo) incorrect. Basically, we would sometimes have abstract array lengths that we can only determine precisely at trans time (but we may still know that two array lengths are the same, even if we don't know what they are yet).

oli-obk · 2016-01-06T16:12:33Z

That's not the issue. The issue is things like addresses to statics which can't be handled. But you are right. My argument is circular.

To the original question. I cannot see a way this escapes a feature gate.

oli-obk · 2016-01-06T16:14:54Z

src/test/compile-fail/associated-const-type-parameter-arrays.rs

-    <T as Foo>::MIN //~ ERROR E0329
+pub fn test<A: Foo, B: Foo>() {
+    let _array: [u32; <A as Foo>::Y] = //~ error: the parameter type
+        [4; <A as Foo>::Y];


Does the previous error prevent the repeat expr from emitting an error? If so, this probably needs a second test

nikomatsakis · 2016-01-08T19:55:59Z

OK, I'm inclined to r+ for now I think, @michaelwu can you produce a rebased version that addresses the nits and answers various questions?

michaelwu · 2016-01-09T15:34:58Z

Will do next week - on PTO right now.

michaelwu · 2016-01-14T02:48:56Z

I've also rebased and squashed, and am building and testing now. These commits are probably easier to check for now.

michaelwu · 2016-01-14T03:58:47Z

Pushed the rebase now since it looks like travis isn't happy.

nikomatsakis · 2016-01-14T18:15:18Z

Ah, I forgot about that trait selection thing. Let me look at that a bit more closely.

nikomatsakis · 2016-01-14T18:46:17Z

OK, so, I think that converting from span_bug to None looks correct. I don't see any tests of what happens when you have an invalid assoc const references, but it does seem to fail during typeck (e.g., with this test I wrote:

// 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)]

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);
}

it'd be great to make more tests of this ilk, I couldn't find any. e.g., using such constants in "early resolution" contexts, like a match arm.

nikomatsakis · 2016-01-14T18:52:23Z

@michaelwu would you mind adding that test I pasted above into your PR, as well as the following one:

// 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)]

trait Foo {
    const ID: usize;
}

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

fn main() {
    assert_eq!(1, X);
}

(I know it's sort of a pre-existing problem and not the fault of your PR, but it makes me feel better about that diff. ;)

michaelwu · 2016-01-14T22:39:14Z

No problem - I've added the tests. I bumped the copyright year for both of them and modified the 2nd test a bit since it didn't look quite right and produced 4 errors. (<i32 as ID>::usize -> <i32 as Foo>::ID)

nikomatsakis · 2016-01-15T20:27:54Z

@bors r+

bors · 2016-01-15T20:27:54Z

📌 Commit a4f91e5 has been approved by nikomatsakis

nikomatsakis · 2016-01-15T20:28:06Z

Thanks @michaelwu (and @oli-obk for the feedback)

bors · 2016-01-15T21:33:59Z

⌛ Testing commit a4f91e5 with merge 683af0d...

@eddyb

…nikomatsakis This provides limited support for using associated consts on type parameters. It generally works on things that can be figured out at trans time. This doesn't work for array lengths or match arms. I have another patch to make it work in const expressions. CC @eddyb @nikomatsakis

bors · 2016-01-15T23:26:37Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned pnkfelix Dec 17, 2015

michaelwu force-pushed the associated-const-type-params-pt1 branch from 87ddada to 7ff4adf Compare December 17, 2015 20:41

rust-highfive assigned nikomatsakis and unassigned pnkfelix Dec 18, 2015

nikomatsakis reviewed Jan 4, 2016
View reviewed changes

oli-obk reviewed Jan 6, 2016
View reviewed changes

michaelwu force-pushed the associated-const-type-params-pt1 branch from a3e8e9a to ba48182 Compare January 14, 2016 03:57

michaelwu force-pushed the associated-const-type-params-pt1 branch from ba48182 to 650345e Compare January 14, 2016 22:35

Support generic associated consts

a4f91e5

michaelwu force-pushed the associated-const-type-params-pt1 branch from 650345e to a4f91e5 Compare January 14, 2016 22:36

bors merged commit a4f91e5 into rust-lang:master Jan 15, 2016

bors mentioned this pull request Jan 15, 2016

simplify some code #30934

Merged

michaelwu mentioned this pull request Jan 12, 2017

Associated consts cannot depend on type parameters or Self #26524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support generic associated consts #30446

Support generic associated consts #30446

michaelwu commented Dec 17, 2015

rust-highfive commented Dec 17, 2015

michaelwu commented Dec 17, 2015

oli-obk commented Dec 18, 2015

michaelwu commented Dec 18, 2015

nikomatsakis commented Dec 18, 2015

nikomatsakis commented Dec 18, 2015

nikomatsakis Jan 4, 2016

michaelwu Jan 14, 2016

nikomatsakis Jan 14, 2016

nikomatsakis commented Jan 4, 2016

oli-obk commented Jan 5, 2016

nikomatsakis commented Jan 5, 2016

oli-obk commented Jan 6, 2016

nikomatsakis commented Jan 6, 2016

oli-obk commented Jan 6, 2016

oli-obk Jan 6, 2016

nikomatsakis commented Jan 8, 2016

michaelwu commented Jan 9, 2016

michaelwu commented Jan 14, 2016

michaelwu commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

michaelwu commented Jan 14, 2016

nikomatsakis commented Jan 15, 2016

bors commented Jan 15, 2016

nikomatsakis commented Jan 15, 2016

bors commented Jan 15, 2016

bors commented Jan 15, 2016

Support generic associated consts #30446

Support generic associated consts #30446

Conversation

michaelwu commented Dec 17, 2015

rust-highfive commented Dec 17, 2015

michaelwu commented Dec 17, 2015

oli-obk commented Dec 18, 2015

michaelwu commented Dec 18, 2015

nikomatsakis commented Dec 18, 2015

nikomatsakis commented Dec 18, 2015

nikomatsakis Jan 4, 2016

Choose a reason for hiding this comment

michaelwu Jan 14, 2016

Choose a reason for hiding this comment

nikomatsakis Jan 14, 2016

Choose a reason for hiding this comment

nikomatsakis commented Jan 4, 2016

oli-obk commented Jan 5, 2016

nikomatsakis commented Jan 5, 2016

oli-obk commented Jan 6, 2016

nikomatsakis commented Jan 6, 2016

oli-obk commented Jan 6, 2016

oli-obk Jan 6, 2016

Choose a reason for hiding this comment

nikomatsakis commented Jan 8, 2016

michaelwu commented Jan 9, 2016

michaelwu commented Jan 14, 2016

michaelwu commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

michaelwu commented Jan 14, 2016

nikomatsakis commented Jan 15, 2016

bors commented Jan 15, 2016

nikomatsakis commented Jan 15, 2016

bors commented Jan 15, 2016

bors commented Jan 15, 2016