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

subtask of RFC 195: Implement associated constants #17841

Closed
pnkfelix opened this issue Oct 7, 2014 · 5 comments · Fixed by #23606
Closed

subtask of RFC 195: Implement associated constants #17841

pnkfelix opened this issue Oct 7, 2014 · 5 comments · Fixed by #23606
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Oct 7, 2014

As a subtask of #17307, we want to implement associated constants at some point.

@aturon notes here that it probably need not block 1.0

This blocks the code revisions implied by #17825.

@seanmonstar
Copy link
Contributor

Would this include constants in implicit implementations, or only when implementing a trait?

@steveklabnik steveklabnik added the B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. label Jan 29, 2015
@pnkfelix pnkfelix changed the title subtask of RFC 59: Implement associated constants subtask of RFC 195: Implement associated constants Feb 16, 2015
@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

@quantheory has done most of the work necessary in their associated constant branch, but it will not be trivial to rebase on top of #23265.

I think we should try and split those changes into "structural" (needed just because there's one more variant of impl/trait items) and "functional" (typeck::check, const_eval, check_consts, etc.).
(this can be done using git rebase -i and git gui or something similar to move parts of each commit into a second one)

After that, we'd have to redo the "structural" changes - I have most of those on top of my refactoring, but I'm not sure how well they'll fit.
@quantheory Is this good with you? How do you think we should split the workload? (I hope I'm not stepping on toes here, just want this done soon)

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

cc @japaric @nikomatsakis (also see rust-lang/rfcs#865).
Relevant for numeric constants after #23104 and removing the modules they reside in.

@quantheory
Copy link
Contributor

@eddyb I'd be interested in seeing what you have for the structural stuff. This is all (relatively) fresh in my mind, so I can tell what almost every change is doing (and whether or not we have a good reason to keep it at all) just by looking. Rather than first splitting the history into separate commits, and then doing a rebase onto #23265 as a separate step, I am tempted to rewrite the history into 2 themed commits and rebase onto #23265 at the same time (basically by hand). If you already have a structural commit more-or-less done, this would be simpler.

I think that I can get this done in a few hours here and there over the next couple of days. If I can get the rebase out of the way, we can split the remaining work just by looking at test cases. I have a significant list of things that I would expect to work, but haven't tried yet, and another list of things that need to produce appropriate error messages rather than ICEs. We can probably do some of those things in follow-up after the initial PR, but a few changes really should be addressed up front (like importing constants from another crate).

@quantheory
Copy link
Contributor

Well, it took a little longer than I was hoping, but my branch is now rebased onto @eddyb's commit. (Of course it's still fallen a bit behind master in the meantime.) I still don't have cross-crate usage working. More info soon as I get some more tests done.

bors added a commit that referenced this issue Apr 27, 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants