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

associated constants should support references to outer type parameters #28809

Closed
pnkfelix opened this Issue Oct 2, 2015 · 9 comments

Comments

Projects
None yet
8 participants
@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2015

associated constants should support references to outer type parameters

Here is an example, adapted from the Zeroable trait from core::non_zero (playpen):

#![feature(associated_consts)]
trait Zble { const Z: Self; }
impl Zble for i32 { const Z: i32 = 0; }

impl<T> Zble for *const T { const Z: *const T = ::std::ptr::null(); }

fn main() { }

An attempt to compile this yields the error:

<anon>:5:45: 5:46 error: cannot use an outer type parameter in this context [E0402]
<anon>:5 impl<T> Zble for *const T { const Z: *const T = ::std::ptr::null(); }
                                                     ^
<anon>:5:45: 5:46 error: use of undeclared type name `T` [E0412]
<anon>:5 impl<T> Zble for *const T { const Z: *const T = ::std::ptr::null(); }
                                                     ^
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 2, 2015

Oh, I was interested in this some time ago, but mostly in Self and not type parameters, which is similar nonetheless.
The changes in librustc_resolve required to resolve Self or type parameters in types or initializers of associated constants are simple, I can do them, but I have no idea what repercussions it will cause in later stages of compilation (especially for initializers).

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Oct 20, 2015

sigh: my actual goal here was to be able to adjust NonZero::new(inner) to assert that inner is non-zero.

I thought I could still achieve this by adding an fn is_zero(&self) -> bool to the trait Zeroable.

And until recently, that worked.

But now with #29085, adding the above check does not work, because we cannot call the non-const is_zero method from the now const function NonZero::new.

And we cannot make fn is_zero itself const, because RFC 911 does not allow trait methods to be declared const.

(I'm not 100% clear about the reasoning there, I have read the discussion in its alternatives section a couple times but still do not quite grok why it is hard to apply the same analysis we use for normal const fn onto the const fn for the impl of the trait...)


Update: okay, clearly even if we got the capability to declare trait methods as const, I would still have the same problem that I could not include the assertion I want in NonZero::new, since as a const fn I would not be allowed to write:

        pub unsafe const fn new(inner: T) -> NonZero<T> {
            debug_assert!(!inner.is_zero());
            NonZero(inner)
        }

meh.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Jan 29, 2016

I just encountered another case where this bug bit me: we have some places where we have a new-type wrapper around a usize acting as an index into some other array-based data-structure, and we use usize::MAX to represent an invalid index.

  • (Thinking on the matter now, I guess there are other options here, like making the array 1-indexed, implementing NonZero for the wrapper, and then using Option<Wrapper> as the actual type with None as the invalid case. But I digress.)

Anyway, I thought I might play with trying to make a trait to capture this pattern of new-typed wrapper around a simple usize index into some associated array container. So I tried making this:

trait Idx {
    type Data;
    const INVALID: Self;
    fn idx(&self) -> usize;
}

then I tried making this impl of the above trait:

pub struct MovePathIndex(usize);

const INVALID_MOVE_PATH_INDEX: MovePathIndex = MovePathIndex(usize::MAX);

impl Idx for MovePathIndex {
    type Data = MovePath;
    const INVALID: Self = INVALID_MOVE_PATH_INDEX;
    fn idx(&self) -> usize { self.0 }
}

and Bam: I hit the bug:

/Users/fklock/Dev/Mozilla/rust-mir/src/librustc_borrowck/borrowck/mir/gather_moves.rs:24:20: 24:24 error: cannot use an outer type parameter in this context [E0402]
/Users/fklock/Dev/Mozilla/rust-mir/src/librustc_borrowck/borrowck/mir/gather_moves.rs:24     const INVALID: Self = INVALID_MOVE_PATH_INDEX;
                                                                                                            ^~~~
/Users/fklock/Dev/Mozilla/rust-mir/src/librustc_borrowck/borrowck/mir/gather_moves.rs:24:20: 24:24 error: use of `Self` outside of an impl or trait [E0411]
/Users/fklock/Dev/Mozilla/rust-mir/src/librustc_borrowck/borrowck/mir/gather_moves.rs:24     const INVALID: Self = INVALID_MOVE_PATH_INDEX;
                                                                                                            ^~~~

(I suspect this is exactly the kind of case that @petrochenkov was talking about above.)

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Jan 29, 2016

(nominating so that the compiler team can decide what priority it is to fix this, assuming it should be fixed in the first place...)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 4, 2016

triage: P-medium

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented May 11, 2016

Hit this just now.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented May 11, 2016

Similar thing you cannot do is

impl<T> Trait for Option<T> {
const C: Option<T> ... = Some(<T as Trait>::C);
                               ^ cannot use T here either
@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented May 11, 2016

cc @eddyb

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented May 11, 2016

The problem here is that we use a constant Rib for associated constants but that is there only to prevent const items inside fns from getting weird errors about accesses to variables which look like they are in scope.
We should just use the same thing for methods and associated consts. Probably not a big diff.

eddyb added a commit to eddyb/rust that referenced this issue May 12, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

eddyb added a commit to eddyb/rust that referenced this issue May 12, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

eddyb added a commit to eddyb/rust that referenced this issue May 12, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

eddyb added a commit to eddyb/rust that referenced this issue May 12, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

eddyb added a commit to eddyb/rust that referenced this issue May 13, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

eddyb added a commit to eddyb/rust that referenced this issue May 13, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

Manishearth added a commit to Manishearth/rust that referenced this issue May 14, 2016

Rollup merge of rust-lang#33572 - nagisa:assoc-const-types, r=eddyb
Support references to outer type params for assoc consts

Fixes rust-lang#28809

r? @eddyb

@bors bors closed this in #33572 May 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.