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

Implement associated constants #23606

Merged
merged 8 commits into from Apr 27, 2015

Conversation

Projects
None yet
@quantheory
Contributor

quantheory commented Mar 22, 2015

Closes #17841.

The majority of the work should be done, e.g. trait and inherent impls, different forms of UFCS syntax, defaults, and cross-crate usage. It's probably enough to replace the constants in f32, i8, and so on, or close to good enough.

There is still some significant functionality missing from this commit:

  • Associated consts can't be used in match patterns at all. This is simply because I haven't updated the relevant bits in the parser or resolve, but it's probably not hard to get working.
  • Since you can't select an impl for trait-associated consts until partway through type-checking, there are some problems with code that assumes that you can check constants earlier. Associated consts that are not in inherent impls cause ICEs if you try to use them in array sizes or match ranges. For similar reasons, check_static_recursion doesn't check them properly, so the stack goes ka-blooey if you use an associated constant that's recursively defined. That's a bit trickier to solve; I'm not entirely sure what the best approach is yet.
  • Dealing with consts associated with type parameters will raise some new issues (e.g. if you have a T: Int type parameter and want to use <T>::ZERO). See rust-lang/rfcs#865.
  • Unused associated consts don't seem to trigger the dead_code lint when they should. Probably easy to fix.

Also, this is the first time I've been spelunking in rustc to such a large extent, so I've probably done some silly things in a couple of places.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Mar 22, 2015

Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Mar 22, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Mar 22, 2015

Member

🎉 This is great!
I'll be looking over the changes, but I'd rather have someone more knowledgeable (e.g. @nikomatsakis) with the latest advances in type-checking do the actual review.

Member

eddyb commented Mar 22, 2015

🎉 This is great!
I'll be looking over the changes, but I'd rather have someone more knowledgeable (e.g. @nikomatsakis) with the latest advances in type-checking do the actual review.

ref_id: ast::NodeId)
-> Option<&'tcx Expr>
{
let rcvr_substs = ty::node_id_item_substs(tcx, ref_id).substs;

This comment has been minimized.

@eddyb

eddyb Mar 22, 2015

Member

So you're expecting substitutions to be there, which means this can't be used earlier...
But this is an ExprPath, isn't it? Which means that at least the <T as Trait>::CONST case could be handled here, if an astconv interface were provided.

That reminds me, I meant to check if it's possible to move const_eval and everything depending on it to rustc_typeck and integrate it properly.
Maybe this can all be done in a follow-up PR after the groundwork has been laid.

@eddyb

eddyb Mar 22, 2015

Member

So you're expecting substitutions to be there, which means this can't be used earlier...
But this is an ExprPath, isn't it? Which means that at least the <T as Trait>::CONST case could be handled here, if an astconv interface were provided.

That reminds me, I meant to check if it's possible to move const_eval and everything depending on it to rustc_typeck and integrate it properly.
Maybe this can all be done in a follow-up PR after the groundwork has been laid.

This comment has been minimized.

@quantheory

quantheory Mar 22, 2015

Contributor

Good point; I'm not really sure. The traits code is probably the main bit that I don't understand as well as I should here, so here (and even more so in rustc_typeck::check::compare_method) there may be a couple of details that are just superstitiously copied.

One thing that concerns me is that maybe we could have a path like <<T>::U as Trait>::CONST or <[T; <U>::N] as Trait>::CONST (or the nested UFCS could be in the trait, too). The former is disallowed right now because <T>::U can't be used to refer to an associated type; I don't know if that ever will be allowed in the future. The latter is of course one of the things I don't have working yet, but it seems like it could be done.

As for moving const_eval to rustc_typeck, probably the main obstacle there is that rustc_trans uses it. It's kind of awkward because middle::const_eval and trans::consts kind of duplicate some of each others' logic, but the former is more broadly used. But I think that const_eval is doing some things wrong that are done right in trans::consts. E.g. in const_eval all floats are treated as f64, and compare_const_vals treats them as totally ordered.

@quantheory

quantheory Mar 22, 2015

Contributor

Good point; I'm not really sure. The traits code is probably the main bit that I don't understand as well as I should here, so here (and even more so in rustc_typeck::check::compare_method) there may be a couple of details that are just superstitiously copied.

One thing that concerns me is that maybe we could have a path like <<T>::U as Trait>::CONST or <[T; <U>::N] as Trait>::CONST (or the nested UFCS could be in the trait, too). The former is disallowed right now because <T>::U can't be used to refer to an associated type; I don't know if that ever will be allowed in the future. The latter is of course one of the things I don't have working yet, but it seems like it could be done.

As for moving const_eval to rustc_typeck, probably the main obstacle there is that rustc_trans uses it. It's kind of awkward because middle::const_eval and trans::consts kind of duplicate some of each others' logic, but the former is more broadly used. But I think that const_eval is doing some things wrong that are done right in trans::consts. E.g. in const_eval all floats are treated as f64, and compare_const_vals treats them as totally ordered.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory
Contributor

quantheory commented Mar 22, 2015

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 23, 2015

Contributor

Exciting. I'll take a look. I agree that the interplay of constants, type-checking, and traits is subtle-- it's going to wind up (I think) as a kind of best-effort thing in the end, though I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known. This is probably related to some refactoring I've been doing for doing better normalization, but anyway.

Contributor

nikomatsakis commented Mar 23, 2015

Exciting. I'll take a look. I agree that the interplay of constants, type-checking, and traits is subtle-- it's going to wind up (I think) as a kind of best-effort thing in the end, though I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known. This is probably related to some refactoring I've been doing for doing better normalization, but anyway.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 24, 2015

Contributor

@nikomatsakis

[...] I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known. This is probably related to some refactoring I've been doing for doing better normalization, but anyway.

I don't quite understand what you're saying. Certainly I'm interested in interaction between constants and types (note that I proposed rust-lang/rfcs#884, which is necessary, though far from sufficient, for some concrete applications I have in mind). Are you saying that you have a justification for deferring evaluation until trans, or that you want to know would I say about this? (I do think that such a change needs to happen, at least in some cases, but it's not directly relevant to this PR, I believe.)

Contributor

quantheory commented Mar 24, 2015

@nikomatsakis

[...] I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known. This is probably related to some refactoring I've been doing for doing better normalization, but anyway.

I don't quite understand what you're saying. Certainly I'm interested in interaction between constants and types (note that I proposed rust-lang/rfcs#884, which is necessary, though far from sufficient, for some concrete applications I have in mind). Are you saying that you have a justification for deferring evaluation until trans, or that you want to know would I say about this? (I do think that such a change needs to happen, at least in some cases, but it's not directly relevant to this PR, I believe.)

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Mar 24, 2015

Member

though I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known

It seems this may lead to old-transmute like errors, where some constant evaluation that is only known in trans is invalid (e.g. depending how we handle it, overflow).

Member

huonw commented Mar 24, 2015

though I'd like to see add some amount of reasoning for treating constants more "generically" and deferring the actual evaluation until trans when types are known

It seems this may lead to old-transmute like errors, where some constant evaluation that is only known in trans is invalid (e.g. depending how we handle it, overflow).

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 24, 2015

Contributor

☔️ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Mar 24, 2015

☔️ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 25, 2015

Contributor

I have gotten match patterns working, though using associated consts in range patterns causes the same ICE as array sizes. (Actually, if we deferred checking that the lower bound is below the upper bound until check_match, I think that that would solve the issue, but some things need to get moved around for array sizes anyway, so I'm not sure if it's worth it.)

I will try to rebase today.

Contributor

quantheory commented Mar 25, 2015

I have gotten match patterns working, though using associated consts in range patterns causes the same ICE as array sizes. (Actually, if we deferred checking that the lower bound is below the upper bound until check_match, I think that that would solve the issue, but some things need to get moved around for array sizes anyway, so I'm not sure if it's worth it.)

I will try to rebase today.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 26, 2015

Contributor

Are you saying that you have a justification for deferring evaluation until trans, or that you want to know would I say about this? (I do think that such a change needs to happen, at least in some cases, but it's not directly relevant to this PR, I believe.)

I wasn't really saying either of those things. Well, I do have justifications for potentially deferring evaluation until monomorphization -- e.g., type-checking [i32; X::LEN] -- but I wasn't suggesting it was relevant to this PR.

Anyway, sorry for the delay. I intend to review today (actually, yesterday, but a lot of stuff came up and I wound up away from my computer for much of the day).

Contributor

nikomatsakis commented Mar 26, 2015

Are you saying that you have a justification for deferring evaluation until trans, or that you want to know would I say about this? (I do think that such a change needs to happen, at least in some cases, but it's not directly relevant to this PR, I believe.)

I wasn't really saying either of those things. Well, I do have justifications for potentially deferring evaluation until monomorphization -- e.g., type-checking [i32; X::LEN] -- but I wasn't suggesting it was relevant to this PR.

Anyway, sorry for the delay. I intend to review today (actually, yesterday, but a lot of stuff came up and I wound up away from my computer for much of the day).

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 26, 2015

Contributor

It seems this may lead to old-transmute like errors, where some constant evaluation that is only known in trans is invalid (e.g. depending how we handle it, overflow).

I was assuming type-checking would be more conservative than trans, not less.

Contributor

nikomatsakis commented Mar 26, 2015

It seems this may lead to old-transmute like errors, where some constant evaluation that is only known in trans is invalid (e.g. depending how we handle it, overflow).

I was assuming type-checking would be more conservative than trans, not less.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 26, 2015

Contributor

@quantheory

So I've been reading through and so far I'm enjoying what I read -- but one comment. We definitely want a feature-gate on this work before it can land! Can you add a feature-gate for defining a trait with an associated constant? (I think that's probably sufficient; we could add a feature-gate for referencing such a constant as well, but so long as we don't export any public interfaces that define them, that shouldn't be necessary.)

Contributor

nikomatsakis commented Mar 26, 2015

@quantheory

So I've been reading through and so far I'm enjoying what I read -- but one comment. We definitely want a feature-gate on this work before it can land! Can you add a feature-gate for defining a trait with an associated constant? (I think that's probably sufficient; we could add a feature-gate for referencing such a constant as well, but so long as we don't export any public interfaces that define them, that shouldn't be necessary.)

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 26, 2015

Contributor

@nikomatsakis

Done! The feature gate covers both traits and impls, since you can define an inherent associated const.

Contributor

quantheory commented Mar 26, 2015

@nikomatsakis

Done! The feature gate covers both traits and impls, since you can define an inherent associated const.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 27, 2015

Contributor

Hmm, I fixed the last error to get a full make check working again, but it looks like Travis CI hit a glitch. 😞

Contributor

quantheory commented Mar 27, 2015

Hmm, I fixed the last error to get a full make check working again, but it looks like Travis CI hit a glitch. 😞

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 28, 2015

Contributor

☔️ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Mar 28, 2015

☔️ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 29, 2015

Contributor

Rebased. Hmm, is there a way to restart the Travis CI build?

Contributor

quantheory commented Mar 29, 2015

Rebased. Hmm, is there a way to restart the Travis CI build?

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 30, 2015

Contributor

☔️ The latest upstream changes (presumably #23673) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Mar 30, 2015

☔️ The latest upstream changes (presumably #23673) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 30, 2015

Contributor

@quantheory cool, thanks. I read through most of the PR and didn't have any major comments, it all seemed quite nice. But then I got distracted again by the beta deadline and in particular researching and implementing rust-lang/rfcs#1023. Argh. I will look it over again today -- but now that there is a feature-gate, I feel pretty good about landing this before I understand every bit. We can improve over time.

Contributor

nikomatsakis commented Mar 30, 2015

@quantheory cool, thanks. I read through most of the PR and didn't have any major comments, it all seemed quite nice. But then I got distracted again by the beta deadline and in particular researching and implementing rust-lang/rfcs#1023. Argh. I will look it over again today -- but now that there is a feature-gate, I feel pretty good about landing this before I understand every bit. We can improve over time.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Mar 31, 2015

Contributor

@nikomatsakis OK, cool. Just rebased!

Contributor

quantheory commented Mar 31, 2015

@nikomatsakis OK, cool. Just rebased!

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 1, 2015

Contributor

OK, so I was doing some experimentation. I found that the following example ICEs:

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

use std::marker::MarkerTrait;

trait Foo: MarkerTrait {
    const ID: i32;
}

trait Bar: MarkerTrait {
    const ID: i32;
}

impl Foo for i32 {
    const ID: i32 = 1;
}

impl Bar for i32 {
    const ID: i32 = 3;
}

const X: i32 = <i32>::ID;

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

the problem is not super deep, it's just that the "ambiguity reporting" code assumes methods.

Contributor

nikomatsakis commented Apr 1, 2015

OK, so I was doing some experimentation. I found that the following example ICEs:

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

use std::marker::MarkerTrait;

trait Foo: MarkerTrait {
    const ID: i32;
}

trait Bar: MarkerTrait {
    const ID: i32;
}

impl Foo for i32 {
    const ID: i32 = 1;
}

impl Bar for i32 {
    const ID: i32 = 3;
}

const X: i32 = <i32>::ID;

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

the problem is not super deep, it's just that the "ambiguity reporting" code assumes methods.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 1, 2015

Contributor

So I'm basically ready to r+. I'm just wondering whether it makes sense to hold this till after the beta branch is cut this evening.

Contributor

nikomatsakis commented Apr 1, 2015

So I'm basically ready to r+. I'm just wondering whether it makes sense to hold this till after the beta branch is cut this evening.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 1, 2015

Contributor

(Basically, I'm inclined to wait because it seems like landing this patch can only create merge conflicts and other uncertainty, and the feature is feature-gated anyhow. Landing all the critical PRs before the branch deadline is always challenging anyway.)

Contributor

nikomatsakis commented Apr 1, 2015

(Basically, I'm inclined to wait because it seems like landing this patch can only create merge conflicts and other uncertainty, and the feature is feature-gated anyhow. Landing all the critical PRs before the branch deadline is always challenging anyway.)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 1, 2015

Contributor

☔️ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 1, 2015

☔️ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 1, 2015

Contributor

@nikomatsakis The original motivation for trying to get this in before the beta was to get rid of all the functions that deal with constants in Int/Float, and then to get rid of all the modules like std::i64 that just hold constants as well. We could have dodged most of the issues (ICEs and generic code design) by using inherent impls instead of associating the constants with traits. But since #23549 came in a bit earlier and stabilized a bunch more of those constants before the beta, whereas this hasn't landed yet, blegh.

Anyway, I'll try to rebase once more today, but I'm going to be on a trip tomorrow evening and may not be available next week, so I won't be doing much with this for a little while. I'll try to get back to some of these ICEs after that.

Contributor

quantheory commented Apr 1, 2015

@nikomatsakis The original motivation for trying to get this in before the beta was to get rid of all the functions that deal with constants in Int/Float, and then to get rid of all the modules like std::i64 that just hold constants as well. We could have dodged most of the issues (ICEs and generic code design) by using inherent impls instead of associating the constants with traits. But since #23549 came in a bit earlier and stabilized a bunch more of those constants before the beta, whereas this hasn't landed yet, blegh.

Anyway, I'll try to rebase once more today, but I'm going to be on a trip tomorrow evening and may not be available next week, so I won't be doing much with this for a little while. I'll try to get back to some of these ICEs after that.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Apr 3, 2015

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 3, 2015

Contributor

@quantheory yeah, a laudable goal, but the timing for that was very tough. But seriously, this is great work, and I'm very excited to see it land!

Contributor

nikomatsakis commented Apr 3, 2015

@quantheory yeah, a laudable goal, but the timing for that was very tough. But seriously, this is great work, and I'm very excited to see it land!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 3, 2015

Contributor

⌛️ Testing commit 8f2138d with merge 5383cc9...

Contributor

bors commented Apr 3, 2015

⌛️ Testing commit 8f2138d with merge 5383cc9...

bors added a commit that referenced this pull request Apr 3, 2015

Auto merge of #23606 - quantheory:associated_const, r=nikomatsakis
Closes #17841.

The majority of the work should be done, e.g. trait and inherent impls, different forms of UFCS syntax, defaults, and cross-crate usage. It's probably enough to replace the constants in `f32`, `i8`, and so on, or close to good enough.

There is still some significant functionality missing from this commit:

 - ~~Associated consts can't be used in match patterns at all. This is simply because I haven't updated the relevant bits in the parser or `resolve`, but it's *probably* not hard to get working.~~
 - Since you can't select an impl for trait-associated consts until partway through type-checking, there are some problems with code that assumes that you can check constants earlier. Associated consts that are not in inherent impls cause ICEs if you try to use them in array sizes or match ranges. For similar reasons, `check_static_recursion` doesn't check them properly, so the stack goes ka-blooey if you use an associated constant that's recursively defined. That's a bit trickier to solve; I'm not entirely sure what the best approach is yet.
 - Dealing with consts associated with type parameters will raise some new issues (e.g. if you have a `T: Int` type parameter and want to use `<T>::ZERO`). See rust-lang/rfcs#865.
 - ~~Unused associated consts don't seem to trigger the `dead_code` lint when they should. Probably easy to fix.~~

Also, this is the first time I've been spelunking in rustc to such a large extent, so I've probably done some silly things in a couple of places.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 3, 2015

Contributor

💔 Test failed - auto-mac-32-opt

Contributor

bors commented Apr 3, 2015

💔 Test failed - auto-mac-32-opt

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 3, 2015

Contributor

Oopsies. I had that easy fix, but I forgot to push yesterday.

Contributor

quantheory commented Apr 3, 2015

Oopsies. I had that easy fix, but I forgot to push yesterday.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 3, 2015

Contributor

☔️ The latest upstream changes (presumably #23938) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 3, 2015

☔️ The latest upstream changes (presumably #23938) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 8, 2015

Contributor

bump

Make check has to run overnight for me due to compile times, and with my new schedule I only have time for compiler stuff a couple of times a week. Since other PRs cause merge conflicts every 1-2 days (even outside of the rush for the beta), I can't rebase often enough to keep this in a mergeable state. It would be good to either just get this merged or get help from someone else to update it.

Contributor

quantheory commented Apr 8, 2015

bump

Make check has to run overnight for me due to compile times, and with my new schedule I only have time for compiler stuff a couple of times a week. Since other PRs cause merge conflicts every 1-2 days (even outside of the rush for the beta), I can't rebase often enough to keep this in a mergeable state. It would be good to either just get this merged or get help from someone else to update it.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 8, 2015

Contributor

☔️ The latest upstream changes (presumably #23998) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 8, 2015

☔️ The latest upstream changes (presumably #23998) made this pull request unmergeable. Please resolve the merge conflicts.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 10, 2015

Contributor

☔️ The latest upstream changes (presumably #24177) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 10, 2015

☔️ The latest upstream changes (presumably #24177) made this pull request unmergeable. Please resolve the merge conflicts.

@cristicbz

This comment has been minimized.

Show comment
Hide comment
@cristicbz

cristicbz Apr 13, 2015

Contributor

What happened to this PR---it was mergeable 10 days ago and @quantheory bumped it five days ago, but it's still not merged :(. I was really looking forward to this change.

Contributor

cristicbz commented Apr 13, 2015

What happened to this PR---it was mergeable 10 days ago and @quantheory bumped it five days ago, but it's still not merged :(. I was really looking forward to this change.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 14, 2015

Contributor

FWIW, at this moment a clean merge is probably still possible since I pushed a fix a couple of days ago. I assumed that @nikomatsakis is busy with a couple of the other PRs that he took over, though an r+ (or some kind of update) would still be appreciated.

Contributor

quantheory commented Apr 14, 2015

FWIW, at this moment a clean merge is probably still possible since I pushed a fix a couple of days ago. I assumed that @nikomatsakis is busy with a couple of the other PRs that he took over, though an r+ (or some kind of update) would still be appreciated.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 14, 2015

Contributor

☔️ The latest upstream changes (presumably #24261) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 14, 2015

☔️ The latest upstream changes (presumably #24261) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 14, 2015

Contributor

@quantheory sorry, looks like a clean merge won't work anymore. I don't tend to notice GH notifications in "real time" unfortunately, but I'll try to keep an eye on this PR and get it landed. If you like I could possibly try to rebase it myself if you find yourself short of time.

Contributor

nikomatsakis commented Apr 14, 2015

@quantheory sorry, looks like a clean merge won't work anymore. I don't tend to notice GH notifications in "real time" unfortunately, but I'll try to keep an eye on this PR and get it landed. If you like I could possibly try to rebase it myself if you find yourself short of time.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 14, 2015

Contributor

@nikomatsakis You can give it a try now; I haven't run all the tests yet, but at least it builds, and the conflict seemed pretty straightforward (a lot of lines getting touched in probe.rs that just happened to be next to my changes).

It's not so much the total amount of time that it takes to keep things up to date, it's more that it's kind of sporadic; sometimes it just takes me a few days to be able to look into a new issue. For instance, I only rebased today because my car's getting work, and they have wifi in the waiting room. I'm also just getting a bit antsy because I want to work on fixing some of the tricky bits here, and move on to maybe trying to do something with dependent types, but I'm hesitant to add any more substantive changes on top of this until it gets merged.

Contributor

quantheory commented Apr 14, 2015

@nikomatsakis You can give it a try now; I haven't run all the tests yet, but at least it builds, and the conflict seemed pretty straightforward (a lot of lines getting touched in probe.rs that just happened to be next to my changes).

It's not so much the total amount of time that it takes to keep things up to date, it's more that it's kind of sporadic; sometimes it just takes me a few days to be able to look into a new issue. For instance, I only rebased today because my car's getting work, and they have wifi in the waiting room. I'm also just getting a bit antsy because I want to work on fixing some of the tricky bits here, and move on to maybe trying to do something with dependent types, but I'm hesitant to add any more substantive changes on top of this until it gets merged.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 14, 2015

Contributor

Oops, just noticed that I'd forgotten to push something again. Now it should build.

Contributor

quantheory commented Apr 14, 2015

Oops, just noticed that I'd forgotten to push something again. Now it should build.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 15, 2015

Contributor

☔️ The latest upstream changes (presumably #24433) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 15, 2015

☔️ The latest upstream changes (presumably #24433) made this pull request unmergeable. Please resolve the merge conflicts.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 22, 2015

Contributor

☔️ The latest upstream changes (presumably #24674) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 22, 2015

☔️ The latest upstream changes (presumably #24674) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory
Contributor

quantheory commented Apr 25, 2015

@cristicbz

This comment has been minimized.

Show comment
Hide comment
@cristicbz

cristicbz Apr 25, 2015

Contributor

@quantheory try #rust-internals IRC?

Contributor

cristicbz commented Apr 25, 2015

@quantheory try #rust-internals IRC?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 27, 2015

Contributor

@bors r+ p=1 4c0ac6d

Giving p=1 due to the fact that this has been hanging around. Thanks @quantheory for your persistence.

Contributor

nikomatsakis commented Apr 27, 2015

@bors r+ p=1 4c0ac6d

Giving p=1 due to the fact that this has been hanging around. Thanks @quantheory for your persistence.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 27, 2015

Contributor

📌 Commit 4c0ac6d has been approved by nikomatsakis

Contributor

bors commented Apr 27, 2015

📌 Commit 4c0ac6d has been approved by nikomatsakis

bors added a commit that referenced this pull request Apr 27, 2015

Auto merge of #23606 - quantheory:associated_const, r=nikomatsakis
Closes #17841.

The majority of the work should be done, e.g. trait and inherent impls, different forms of UFCS syntax, defaults, and cross-crate usage. It's probably enough to replace the constants in `f32`, `i8`, and so on, or close to good enough.

There is still some significant functionality missing from this commit:

 - ~~Associated consts can't be used in match patterns at all. This is simply because I haven't updated the relevant bits in the parser or `resolve`, but it's *probably* not hard to get working.~~
 - Since you can't select an impl for trait-associated consts until partway through type-checking, there are some problems with code that assumes that you can check constants earlier. Associated consts that are not in inherent impls cause ICEs if you try to use them in array sizes or match ranges. For similar reasons, `check_static_recursion` doesn't check them properly, so the stack goes ka-blooey if you use an associated constant that's recursively defined. That's a bit trickier to solve; I'm not entirely sure what the best approach is yet.
 - Dealing with consts associated with type parameters will raise some new issues (e.g. if you have a `T: Int` type parameter and want to use `<T>::ZERO`). See rust-lang/rfcs#865.
 - ~~Unused associated consts don't seem to trigger the `dead_code` lint when they should. Probably easy to fix.~~

Also, this is the first time I've been spelunking in rustc to such a large extent, so I've probably done some silly things in a couple of places.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 27, 2015

Contributor

⌛️ Testing commit 4c0ac6d with merge 857ef6e...

Contributor

bors commented Apr 27, 2015

⌛️ Testing commit 4c0ac6d with merge 857ef6e...

@bors bors merged commit 4c0ac6d into rust-lang:master Apr 27, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@quantheory

This comment has been minimized.

Show comment
Hide comment
@quantheory

quantheory Apr 27, 2015

Contributor

🎉

Contributor

quantheory commented Apr 27, 2015

🎉

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Apr 27, 2015

Contributor

Congrats.
Now it's time to empty the "primitive" modules!

Contributor

petrochenkov commented Apr 27, 2015

Congrats.
Now it's time to empty the "primitive" modules!

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark May 6, 2015

Contributor

Thanks @quantheory, this is a huge boon for us!

Contributor

kvark commented May 6, 2015

Thanks @quantheory, this is a huge boon for us!

@burdges

This comment has been minimized.

Show comment
Hide comment
@burdges

burdges Oct 12, 2016

Anyone considered replacing mem::size_of::<T>() with an associated constant, maybe part of the Sized trait or maybe not?

burdges commented Oct 12, 2016

Anyone considered replacing mem::size_of::<T>() with an associated constant, maybe part of the Sized trait or maybe not?

@mitchmindtree

This comment has been minimized.

Show comment
Hide comment
@mitchmindtree

mitchmindtree Oct 12, 2016

Contributor

@burdges I think this is one of the examples given in the original RFC

Contributor

mitchmindtree commented Oct 12, 2016

@burdges I think this is one of the examples given in the original RFC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment