Skip to content

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 24, 2017

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2017
@eddyb
Copy link
Member

eddyb commented Oct 24, 2017

Can you add a test, with an associated const forwarding to another, generically?
That sort of thing wouldn't work without this change.

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 24, 2017

Can you add a test, with an associated const forwarding to another, generically?
That sort of thing wouldn't work without this change.

I tried. But it was a silent error before, and it works now, so there's no difference. Using the assoc constant won't work generically out of other reasons: https://play.rust-lang.org/?gist=54e162b150e745236878f2f0eca06de3&version=stable

@eddyb
Copy link
Member

eddyb commented Oct 24, 2017

@oli-obk There's no forwarding going on there between two impls' associated consts - and you can't have array lengths that depend on type parameters yet.

Closer to what I meant: https://play.rust-lang.org/?gist=e0ec487c9566ea8f8187eb14d82e62ac&version=nightly

@@ -289,6 +267,7 @@ fn eval_const_expr_partial<'a, 'tcx>(cx: &ConstContext<'a, 'tcx>,
match cx.tables.qpath_def(qpath, e.hir_id) {
Def::Const(def_id) |
Def::AssociatedConst(def_id) => {
let substs = tcx.normalize_associated_type_in_env(&substs, cx.param_env);
Copy link
Contributor

@arielb1 arielb1 Oct 24, 2017

Choose a reason for hiding this comment

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

Isn't this normalization also needed in the Def::Method(id) | Def::Fn(id) arm, and also for types fetched from expr_ty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically we only need it for Def::AssociatedConst, but it's a nop for Def::Const

@arielb1
Copy link
Contributor

arielb1 commented Oct 24, 2017

Another test for broken subst logic (this shouldn't ICE):

#![allow(unused)]

trait Foo {
    const BAR: Self;
}

impl Foo for i32 {
    const BAR: Self = 4;
}
impl Foo for i64 {
    const BAR: Self = 3;
}

struct Bar<T: ?Sized>(usize, std::marker::PhantomData<T>);

impl<T: ?Sized> Foo for Bar<T> {
    const BAR: Self = Bar(4, std::marker::PhantomData);
}

impl<T: ?Sized> Bar<T> {
    fn foo() {
        let x = Self::BAR.0;
        let x: &'static usize = &Self::BAR.0;
        let x: [i32; Self::BAR.0] = [1, 2];
    }
}

fn main() {
    let x: [i32; i32::BAR as usize] = [1, 2, 3, 4];
    let x: [i32; i64::BAR as usize] = [1, 2, 3];
}

r+ with proper tests

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 25, 2017

@arielb1 That subst doesn't come from const eval, it comes from https://github.com/rust-lang/rust/blob/master/src/librustc/traits/project.rs#L357

// except according to those terms.

#![feature(const_size_of)]
#![allow(unused)]
Copy link
Member

Choose a reason for hiding this comment

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

You should remove this.

// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(const_size_of)]
Copy link
Member

Choose a reason for hiding this comment

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

Wait, this too, I think? Since the example doesn't use size_of anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 25, 2017

travis likes it + comments addressed

@eddyb
Copy link
Member

eddyb commented Oct 25, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 25, 2017

📌 Commit 1ee0ff3 has been approved by eddyb

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 25, 2017
@bors
Copy link
Collaborator

bors commented Oct 26, 2017

⌛ Testing commit 1ee0ff3 with merge e0febe7...

bors added a commit that referenced this pull request Oct 26, 2017
Resolve types properly in const eval

r? @eddyb

cc @arielb1
@bors
Copy link
Collaborator

bors commented Oct 26, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing e0febe7 to master...

@bors bors merged commit 1ee0ff3 into rust-lang:master Oct 26, 2017
@oli-obk oli-obk deleted the ctfe_resolve branch June 15, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants