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

Resolve types properly in const eval #45488

Merged
merged 2 commits into from
Oct 26, 2017
Merged

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
Contributor

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
Contributor

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
Contributor

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