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

Update RFC 195 to account for RFC 246. #865

Merged
merged 4 commits into from
Jun 18, 2015

Conversation

quantheory
Copy link
Contributor

Edit: While the goal of this RFC is the same, namely to provide further interpretation of the design of associated constants, the current proposal differs radically from the original PR. This PR now clarifies that associated statics are not added, only constants, and mentions some limitations on associated constants that require more design work to deal with.

Rendered.

Original text of this post follows

Currently, the associated items RFC only provides for associated functions, lifetimes, statics, and types. However, it was never updated to account for the const/static split. This PR makes minimal changes to address this by adding associated consts as well.

The original motivating example for statics really applies to consts now, and I have updated it accordingly. I have not added a motivating example for statics; I'm sure that there is a use case, but I have not come up with one that could be easily explained.

I have also not bothered to update the examples to account for other RFCs (e.g. uint is now usize, [T, ..N] is now [T; N]). Maybe that would be worth doing for major RFCs that are going to be used as references into the future, but for now I left the RFC as it was historically accepted, since those other cases are not likely to cause confusion about what was intended.

@Ericson2314
Copy link
Contributor

Do associated statics exist? That might encourage writing associated functions that mutate the associated static. And IMO, any sort of "singleton pattern" like that is evil and should not be encouraged in the slightest.

@tshepang
Copy link
Member

Maybe add a historical note to the RFC, which would help avoid confusion.

@quantheory
Copy link
Contributor Author

@Ericson2314 Right now, neither associated statics nor consts are allowed because the RFC has not been fully implemented. I don't have a strong opinion about whether associated statics should be allowed or not, since I have trouble thinking of any really concrete situation where they would even be useful. I'd really just like the RFC to be clear on this point either way.

@tshepang I've added a brief note in the summary.

@Ericson2314
Copy link
Contributor

@quantheory Thanks for the status confirmation. Totally with you on the "being clear either way" part

@pnkfelix
Copy link
Member

pnkfelix commented Mar 4, 2015

Thank you very much for pointing out this omission!

It is true that RFC 195 was written in a context where we only had static, and this does need to be clarified before we actually implement associated value items (as opposed to the current fn and type items).

So, once we get around to implementing associated constants, then we will need to address this in some way.

However, associated constants are not part of the plan for the 1.0 schedule. Therefore, we are going to mark this particular PR as postponed; see newly filed issue #932.

Again: we definitely want to address this, and I hope that the work you have provided here gets us most or all of the way there. But we want to wait until after 1.0 ships to deal with it.

@pnkfelix pnkfelix closed this Mar 4, 2015
@pnkfelix pnkfelix added the postponed RFCs that have been postponed and may be revisited at a later time. label Mar 4, 2015
@quantheory
Copy link
Contributor Author

@pnkfelix OK. To clarify, I am working on implementing associated constants, which is one reason that I wanted to clarify. But the basic idea seems sufficiently obvious that I don't think I need clarification on this particular point. I think that most people have assumed that associated const would happen, and the outstanding question is whether or not static would happen too.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

@quantheory Well, this is unfortunate to find out about so late... I also am working on associated constants, and have spent a lot of time already refactoring the compiler so associated constants can be added sanely - see rust-lang/rust#23265 - but I was delayed by a week of exams.

How far along is your implementation (if you don't mind me asking)?
Have you made typeck::check::method handle associated constants or did you pick another route?
Also, do you have anything in mind for const_eval?
I'd like to help with rebasing on top of rust-lang/rust#23265, if you already have a lot of changes (it's mostly grunt work and I have to make up for the breakage).

@pnkfelix There was a misunderstanding, associated constants are needed for patching up post-UFCS holes in 1.0.
I've been talking to @alexcrichton and @aturon about this, and they both agree on my plan to solve the arising conflicts, but neither of them seemed to be aware of this RFC (I also wanted to clarify the need for s/static/const/).
@japaric has been working on something similar (marking impls of primitives as lang items), which will also need associated constants for integers and floats.

@quantheory
Copy link
Contributor Author

@eddyb

I should preface this by saying that I had no idea what I was doing when starting out. I was doing this in part to learn how rustc is put together, so quite a lot of my changes involve copy/pasted code that would need to be cleaned up anyway. I will try to take a look at your changes tomorrow and push my latest onto Github so that you can see it. I'm not sure how difficult a rebase will be; it's probably simple but tedious.

I have some basic things working already; the big thing I'm missing is cross-crate usage. I feel like I'm close there, but I'm not sure, since a lot of metadata is still a bit mysterious to me.

Specifically, I've got these working so far:

  • <T as Trait>::N
  • <T>::N (using a modified check::method, as you say)
  • Defaults
  • Constants in inherent impls

Note that the last two are not actually complete for associated types yet, but they are in the RFC.

For const_eval, a lot of the changes were straightforward. I just modified some of the match expressions to handle DefIds for associated constants. IIRC, the one big change is that middle::traits::select is used to resolve trait-associated constants to the appropriate impl value (or the default value).

Constants don't play very nicely with type parameters. I've got it working so that you can have a trait-associated constant of a type that contains type parameters, if there is no default value. That's the minimum necessary for Int and Float, since their associated constants will be declared with type Self. Other than that case, associated constants can't have generic types yet.

Note also that some potential uses of associated constants in generic code will be impossible without moving monomorphization from trans to typeck, e.g.:

unsafe fn interpret_as_bytes<T>(x: T) -> [u8; <T as Sized>::sizeof] { /*... */ }
fn foo<T: Int>(x: T) {
    match x {
        <T>::ZERO => { /* ... */}
       _ => {}
    }
}

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

Well, you seem to be ahead of me - I got stuck on metadata and realized it would be very hard to get anything done with the AST in that sad state.
My intention was to produce a DRY implementation, unifying methods and associated constants - potentially even associated types (those already forming some dead weight, making development of new features harder).

The examples you give are not impossible, merely hard. This one, however, should not compile:

fn foo<T: Int>(x: T) {
    match x {
        <T>::ZERO => { /* ... */ }
        <T>::ONE => { /* ... */ }
       _ => {}
    }
}

That is because match checking can't know those values, so it has to assume they could be equal unless we had a feature where Int would require that Self::ZERO != Self::ONE, but that's a bit into the future.

@quantheory
Copy link
Contributor Author

@eddyb

You can see what I have so far on my associated const branch. Note that there are quite a few things that I want to change. E.g. one of the first things I did was add an ast struct called StaticDef, which really should have been called something like TypedConstExpr and which should probably be taken back out because it doesn't really help anything anyway. Also the commit history is kind of messy.

I think that what I'd like to do is to clean my branch up, and try the rebase myself in the next couple of days. That said, I could use your help with a few things after that.

My point with the earlier examples was that they were impossible without doing certain things earlier than trans. You can make some pretty complex examples involving generic code. I don't think that arrays can be properly type-checked if a constant can depend on an arbitrary constant expression involving a type parameter, unless monomorphization (or part of it) is done during type checking.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

@quantheory you cannot use function monomorphization during type-checking, that violates Rust's functional isolation paradigm.
However, the only thing you need to do is substitute type parameters in expressions, whereas right now they're only substituted in types.

There's a pretty straight-forward way to store the "value" of an expression which cannot be fully evaluated yet, due to the presence of generic type parameters: a "projection" (that's what @nikomatsakis calls it, anyways).
We already use it for associated types, where ty_projection contains the definition of the associated type (from the trait) and substitutions for the trait's Self and type parameters.

For constants, it would have to be a whole expression (AST for now, as we have nothing better), and substitutions for each ExprPath node that references an associated constant from a trait.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

I've taken a look at that first commit, and yes, that's exactly what I did when I first started, as well.
But on top of rust-lang/rust#23265, I now have:

pub enum TraitItem_ {
    MethodTraitItem(MethodSig, Option<P<Block>>),
    TypeTraitItem(TyParamBounds, Option<P<Ty>>),
    ConstTraitItem(P<Ty>, Option<P<Expr>>),
}

pub enum ImplItem_ {
    MethodImplItem(MethodSig, P<Block>),
    TypeImplItem(P<Ty>),
    ConstImplItem(P<Ty>, P<Expr>),
    MacImplItem(Mac),
}

Which I've found to be much easier to work with.
Btw, I don't want to get too deep into implementation details on this issue, could we continue this on IRC?

EDIT: Just looked at the changes to typeck/check/method.rs, it's what I was hoping for, we're so keeping that 😍.

@quantheory
Copy link
Contributor Author

I thought about it a little more, and I guess that I partially agree. At least the examples I mentioned might turn out to be OK. However, you do have to live with some restrictions in the same vein as what's mentioned here.

Specifically, the compiler can't prove that [u8; <T>::N+<T>::N] and [u8; <T>::N*2] are the same type. Therefore either you are stuck rejecting some constant expressions in generic code (that would work if there were no type parameters involved), or you have to throw type errors after monomorphization (whether that means moving monomorphization up or type errors down).

(OK, I just saw your next comment. I'm about to go to work, but we should talk on IRC at some point. In the meantime, if you want to leave notes in a more appropriate place, try rust-lang/rust#17841? I think that your ast changes will be very helpful, since e.g. they accord with my treating defaults for associated constants as simply Option<P<Expr>>, rather than using the Required/Provided type of split.)

@eddyb
Copy link
Member

eddyb commented Mar 12, 2015

"throw type errors after monomorphization" is the part that is not compatible with Rust.
@nikomatsakis can you confirm that the example above should be rejected, lest we normalize one of the expressions to the other? (though that could require a full SMT solver, and that is not something I am eager to have in rustc)

@aturon
Copy link
Member

aturon commented Mar 12, 2015

I'm going to reopen this RFC. Given that multiple people are interested in implementing this feature, and there's not much to think about re: updating the original RFC to talk about const as well, I don't think this RFC needs to be postponed.

@aturon aturon reopened this Mar 12, 2015
@aturon aturon removed the postponed RFCs that have been postponed and may be revisited at a later time. label Mar 12, 2015
@aturon aturon self-assigned this Mar 19, 2015
@quantheory
Copy link
Contributor Author

I hope that it's not too much of a curveball, but I've significantly changed what's here. The basic reasoning is:

  1. Most people (including myself) seem to have assumed that the intention in this RFC was to add associated constants, and not associated (potentially mutable) global variables. So instead of adding associated consts on top of statics, this pull request now simply interprets the original text as referring to constants, and updates it to make that clearer.
  2. I noticed that in generic code, you really can't use associated consts in all of the same situations as you can use other consts. So I added some sections to point out where there are limitations that we don't have a completely "obvious" answer to. That could be a separate RFC, and I'd be willing to make one if it should be its own file. But I think that those sections are really just trying to puzzle out what is or isn't possible within the original design, so in that sense it sort of fits here as a sort of clarification/reinterpretation of the RFC.

const VAR: MyEnum;
}
fn do_something<T: HasVar>(x: MyEnum) {
const y: MyEnum = <T>::VAR;
Copy link
Member

Choose a reason for hiding this comment

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

This is invalid, items cannot depend on type parameters.
You can just use T::VAR instead of y in the match pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right, I forgot to address that. My standpoint is basically that there's not much reason to apply that restriction to constants. It doesn't seem to be "official" in the sense of being mentioned in an RFC (?). It also wasn't applied to statics until rust-lang/rust#9186, and the motivation behind that change is not really applicable to constants, which are inlined.

It looks to me like that constants were forbidden from being dependent on type parameters as a side effect of the restriction on statics (well, they were the same thing back then), and before now having generic constants hasn't actually been useful enough to matter anyway. This match pattern example doesn't require a const to be declared this way, but the latter discussion on array sizes really falls apart without that ability. You basically have to depend on the TypeAdd strategy if you want to do any kind of arithmetic to produce an array size. (Which maybe is OK; dealing with array sizes generically is already kind of icky in ways that associated consts alone won't fix, so it's just more motivation to figure that out.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, plus, if you want to have associated consts depend on type parameters in traits, you already have to do most of the implementation work, and AFAIK, there's no additional reason not to allow it in functions at that point. What I mean is that we presumably expect to allow impls like this:

struct WrapInt<T: Int>(T);

impl<T: Int> Int for WrapInt<T> {
    const ZERO: WrapInt<T> = WrapInt(<T as Int>::ZERO);
    /* ... */
}

(Well, maybe that exact example is not so great in that Int is designed for primitives, but you get the idea.)

Copy link
Member

Choose a reason for hiding this comment

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

Well, the real reason is more serious: nested items cannot see type parameters of their containing scopes.
...and removing this limitation requires HKT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true for functions, perhaps, but I really don't see why this should be the case for constants. Constants can't introduce type parameters of their own, and lifetime parameters would be meaningless anyway since everything must be 'static. So there's no actual nesting here; constants just sit in their containing scope like expressions, AFAICT.

@theemathas
Copy link

Are associated constants object-safe? Is it legal to add where Self: Sized or the like to associated constants?

Note that static methods (aka associated functions) are not object-safe.

@quantheory
Copy link
Contributor Author

No and no. That is, we could probably invent some kind of Sized-related restriction that would allow constants to be emitted into vtables, but AFAIK, no one has requested that. Of course, you couldn't actually use them as constants even if they were allowed. There are no where clauses allowed on associated constants, so there's no way to express such a restriction anyway.

@quantheory
Copy link
Contributor Author

Hmm, to clarify, those were supposed to be two different answers.

  1. Associated consts are never object-safe, in the narrow sense of being safe to use in code that deals with objects.
  2. In order to preserve the object-safety of traits that use them, I really think that we have to treat all constants as if they had an implicit where Self::Sized. Does that make sense?

@eddyb
Copy link
Member

eddyb commented Mar 27, 2015

@quantheory I don't think we should do that, I doubt there are many traits which would use associated constants without being otherwise object unsafe (Int would have methods involving by-value Self).

@quantheory
Copy link
Contributor Author

@eddyb: If so, that's not a reason for one side or the other, so much as an argument that it hardly matters either way.

@theemathas
Copy link

@quantheory This is the specific use-case I had in mind when I was asking: #956 (comment)

@eddyb
Copy link
Member

eddyb commented Mar 27, 2015

@quantheory Sorry, I wasn't clear enough: an implied bound will require weird special-cases to remove later, it's a cost that has to be well justified.
It's better not to assume limitations will stay with the design, i.e. associated constants in vtables have been wanted for a long time.

@quantheory
Copy link
Contributor Author

@eddyb OK, I see now. I think that the requirement would have to be that you can't useSelf (or any Self-associated type) in the constant's type, and the constant also has to be Sized. That seems like it would be enough to avoid issues where you don't know the size of an item in the viable.

Even so, I'd like it better if we could express the where Self: Sized restriction on consts by some means.

@eddyb
Copy link
Member

eddyb commented Mar 27, 2015

@quantheory well, it has to be the same type across all trait objects, so only Self is an issue (associated types have to appear in the trait object type, i.e. Box<Deref<Target=T>> is valid, Box<Deref> isn't).

No constants are unsized right now, and even if we were to support that, we can always put them behind a pointer - after all, they're static.

Adding where clauses to associated constants seems like a sensible thing to me, especially given that they are values, just like methods.

bors added a commit to rust-lang/rust that referenced this pull request 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.
@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 27, 2015
@aturon
Copy link
Member

aturon commented May 27, 2015

I've added the T-lang tags, and am calling for some attention from the @rust-lang/lang subteam. (We're all digging out of quite a backlog from the 1.0 release, still.)

@quantheory
Copy link
Contributor Author

I have not thought about this PR in some time, since the majority of the text is superseded by #1062. I hope it's not too much of a bait-and-switch, but I just erased most of the generic constant discussion in favor of that other RFC, which I think is more detailed and current. At the same time, I did add a mention of trait objects here, simply because it seems much more straightforward and consistent with the original RFC.

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC was moved into Final Comment Period as of Wed, June 10th. (Sorry, forgot to add this notice!)

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 11, 2015
@nikomatsakis
Copy link
Contributor

👍 from me. I agree that "associated constants" are more in the spirit of the original RFC than associated statics. Associated statics in particular seem to raise some thorny questions about precisely when the static is instantiated, since statics have observable identity.

@aturon
Copy link
Member

aturon commented Jun 15, 2015

👍, this is in line with the original intent of the RFC.

I notice that the RFC text is out of date with respect to the scoping rules we ended up with, but that can be dealt with separately.

@nikomatsakis
Copy link
Contributor

It's official... the language subteam has decided to accept this RFC.

nikomatsakis added a commit that referenced this pull request Jun 18, 2015
Update RFC 195 to account for RFC 246.
@nikomatsakis nikomatsakis merged commit bdf52a8 into rust-lang:master Jun 18, 2015
@Centril Centril added A-const Proposals relating to const items A-traits Trait system related proposals & ideas labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const Proposals relating to const items A-traits Trait system related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants