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

Allow fields in traits that map to lvalues in impl'ing type #1546

Closed
wants to merge 5 commits into from

Conversation

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 16, 2016

UPDATE: The finishing touches and final conversation on this RFC are taking place in a dedicated repository, as described in this comment.


The primary change proposed here is to allow fields within traits and then permit access to those fields within generic functions and from trait objects:

  • trait Trait { field: usize }
  • Implementing type can "direct" this field to any lvalue within itself
  • All fields within a trait or its supertraits must be mapped to
    disjoint locations

Fields serve as a better alternative to accessor functions in traits. They are more compatible with Rust's safety checks than accessors, but also more efficient when using trait objects.

Many of the ideas here were originally proposed in #250 in some form. As such, they represent an important "piece of the puzzle" towards solving #349.

cc @eddyb @aturon @rust-lang/lang

Rendered view.

Planned edits

  • Adopt let syntax for declaring fields in traits and impls. (decided against this)
  • Clarify that there are "private type in public API" rules that apply to the type of a field.
  • Decide whether mut is required for mutable access. (At minimum, add as an unresolved question.)
  • Typo
@nikomatsakis nikomatsakis added the T-lang label Mar 16, 2016
@nikomatsakis nikomatsakis self-assigned this Mar 16, 2016
@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2016

#1215 also addresses the borrowck problem with accessors, but still has the poor performance issue and there wasn't any talk about traits (yet). The suggestion was some kind of explicit disjoint partitions (e.g. struct field names) that can be mentioned in references to allow partial borrows.


```rust
trait Trait {
field1: Type1, // <-- fields within a block separated by commas.

This comment has been minimized.

Copy link
@ticki

ticki Mar 16, 2016

Contributor

Another possibility is to declare "fields" (accessors) using a syntax similar to variable declaration:

trait Trait {
    let field1: Type1;
    let field2: Type2;

    fn foo();
}

This also signifies that it isn't really a field, but more an associated value. It also looks nicer in impls.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Mar 16, 2016

Contributor

this syntax is mentioned 10 lines below.

This comment has been minimized.

Copy link
@ticki

ticki Mar 16, 2016

Contributor

Oh, well. I missed that.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 16, 2016

Author Contributor

Another possibility is to declare "fields" (accessors) using a syntax similar to variable declaration:

trait Trait {
    let field1: Type1;
    let field2: Type2;

    fn foo();
}

This also signifies that it isn't really a field, but more an associated value.

Yeah, I don't hate this. It might be better, all things considered. I
like having all trait items start with a keyword, probably helps us
with future expansions of the syntax, and avoids the awkward , vs
; quesiton.

This comment has been minimized.

Copy link
@ticki

ticki Mar 16, 2016

Contributor

It also has a symmetry to type.

- **The ability to index into fixed-length arrays with a constant
index.** However, it would be best to couple that with a general
overhaul of constant evaluation (and probably an extension of
borrowck to understand expressions of this form more broadly).b

This comment has been minimized.

Copy link
@ticki

ticki Mar 16, 2016

Contributor

There is a typo here.

the trait and define a field with that name. Sometimes it may be
necessary or desirable to specify the trait explicitly. For those
cases, we introduce a fully qualified field notation which looks like
`x.<Trait<U,V>::f>`.

This comment has been minimized.

Copy link
@nagisa

nagisa Mar 16, 2016

Contributor

<x as Trait<U, V>>.f?

EDIT: whoops.

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 16, 2016

Member

x is not supposed to be a type, but an expression, so that form wouldn't necessarily be unambiguous.

This comment has been minimized.

Copy link
@nagisa

nagisa Mar 16, 2016

Contributor

Oh wow, I actually came to like the proposed syntax.

This comment has been minimized.

Copy link
@DanielKeep

DanielKeep Mar 17, 2016

Wouldn't the obvious equivalent be (x as Trait<U, V>).f?

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 17, 2016

Member

No, because casts are always rvalues and as at the type level is a completely unrelated construct.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Mar 17, 2016

Member

Might this screw up the grammar? I'm thinking of ambiguities with 1. < foo::CONST >.

This comment has been minimized.

Copy link
@arielb1

arielb1 Mar 18, 2016

Contributor

@Manishearth

No. That works at the tokenizer level.

field path on the left-hand-side of `()`. In other words, just as
calling a closure located at `a.b.c` must be written `(a.b.c)()` (so
as to avoid ambiguity with calling a method `c` on the path `a.b`), so
must you wrote `(a.b.<Trait::c>)()`; `a.b.<Trait::c>()` will not

This comment has been minimized.

Copy link
@nagisa

nagisa Mar 16, 2016

Contributor

s/wrote/write/

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Mar 16, 2016

Contributor

To be clear, the paren restriction could be removed, as no trait can have a field and method with the same name, right?

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 16, 2016

Member

Same for calling direct fields vs methods, if we can properly disambiguate at type-checking time.

This comment has been minimized.

Copy link
@Jexell

Jexell Mar 16, 2016

You could also perhaps give either fields or methods a priority and introduce a lint.

This comment has been minimized.

Copy link
@notriddle

notriddle Mar 17, 2016

Contributor

To be clear, the paren restriction could be removed, as no trait can have a field and method with the same name, right?

Why not?

This comment has been minimized.

Copy link
@Nashenas88

Nashenas88 Mar 18, 2016

If we do leave it in, is this the right place to mention that the existing help span should be extended? There's one in place now that will recognize a field is a closure and rewrites your code with the parenthesis as a suggestion.

`x.<Trait<U,V>::f>`.

This is comparable to the associated item notation `<T as
Trait<U,V>>::foo`, but with some differences. First, the `Self` type

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Mar 16, 2016

Contributor

Type::field/<Type>::field/<Type as Trait>::field is a very attractive syntax for field projection functions (aka pointers to data members):

struct Person { age: u8 }
let opt_age: Option<u8> = opt_person.map(Person::age);

It would be nice to preserve it while selecting syntax for this RFC.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Mar 16, 2016

I like this a lot---it's broadly useful even outside the realm of "virtual structs", and very orthogonal to the listed future work, which are the good smells for this sort of thing.

[Kinda off topic] Another route of generalization is "first class lvalues". I don't know what his would look like, but it might be useful wrt things like map entry API. Teaching the borrow checker that entries for disjoint keys are disjoint would be neat. This is absolutely out of scope for this RFC, but if this is accepted, it opens the door to further exploration in that direction---great!

languages. This means that if, e.g., `Circle` wanted to override the
`highlight` method but also call out to the prior version, it can't
easily do so. Super calls are not strictly needed thoug, as one can
always refactor the super call into a free fn.

This comment has been minimized.

Copy link
@futile

futile Mar 17, 2016

This section contains references to types such as Container and Circle which are not mentioned in the RFC anywhere else. Probably a left-over from previous revisions?

}
impl Trait for Illegal {
x: self.z // ERROR: Private item in public API

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Mar 17, 2016

Contributor

How this is different from

impl Trait for Type {
 fn private_field(&mut self) -> &mut FieldType { &self.private_field }
}

from the privacy point of view?
I wouldn't expect private-in-public checks to be applied to these fields at all.
(Type of the field should be checked though.)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 17, 2016

Author Contributor

@petrochenkov

How this is different from [a fn that references the field]

It feels different to me because I am projecting out the field itself. Thus I have effectively made the field public. In contrast, with a wrapper function, I am projecting out a method that mediates access to the underlying field.

It is roughly the same argument as with an associated type. Both field mappings and associated types give people another way to name something that already exists. Therefore, we have to respect the privacy rules on that underlying thing. In contrast, defining a method defines a new thing which is implicitly public.

If we had "function aliases", where you just map a function directly to another (rather than wrapping it), I would expect the privacy rules to be stricter there as well.

Type of the field should be checked though.

I guess I did not say that explicitly, but I agree.

This comment has been minimized.

Copy link
@rkjnsn

rkjnsn Mar 24, 2016

Contributor

@nikomatsakis I disagree. That there is a field of a given type is part of the public interface exposed through the trait, but I would argue where in the struct it is stored, and what it is named there, could easily be considered an implementation detail. I want to have the flexibility to organize my struct (including grouping fields into member structs) without breaking the public interface, which I can't do if the fields are forced to be public.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 29, 2016

Author Contributor

@rkjnsn

I want to have the flexibility to organize my struct (including grouping fields into member structs) without breaking the public interface, which I can't do if the fields are forced to be public.

This is a strong argument. I've been somewhat reconsidering this point. One difference between the case of fields and the case of associated types is that associated types are true synonyms and normalized, so if we allowed private types to appear in associated type bindings, then we would effectively allow instances of those types to escape into generic functions, which they are not supposed to do.

With fields, somewhat like methods, so long as the type of the field is public, you are only allowing those parts of the value to escape that are designated in the trait. (This was @petrochenkov's original point as well, I believe).

@Manishearth
Copy link
Member

Manishearth commented Mar 17, 2016

Note that as an inheritance system this is not useful for Servo's DOM. This is just a datapoint, not a reason to block it 😄 Otherwise big 👍 , sounds useful.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 17, 2016

Any plans to incorporate mutability in this RFC?
E.g. if I want to emulate a getter function with a trait field, then I wouldn't want this field to be mutable.
Maybe these fields should even be immutable by default.

trait Tr {
    x: u8, // Immutable field access, "getter"
    mut y: u8, // Mutable field access, "setter/getter"
}
@eddyb
Copy link
Member

eddyb commented Mar 17, 2016

@petrochenkov That would be great with fields in inherent impls!
It would mean you can have a private field with an immutable public view in an impl, so you can mutate it, but not anyone else.

@jFransham
Copy link

jFransham commented Mar 17, 2016

Big 👍 on this, I personally would prefer that trait fields are not immutable by default, but that mutability is controlled by whether or not the concrete value is mutable. I think if we can work out a general-case disjointness checking solution (the syntax that comes to mind is fn modstigate(self: &Self { field_a }) -> &TypeOfFieldA) then we could keep the semantics of trait fields and struct fields consistent, and use privacy to control field mutability on both (like we do already).

@kylone
Copy link

kylone commented Mar 17, 2016

@jFransham Just to be clear, "use privacy to control field mutability" means using Cell & RefCell in std::cell, right?

@pnkfelix
Copy link
Member

pnkfelix commented Mar 17, 2016

@Manishearth

Note that as an inheritance system this is not useful for Servo's DOM

Just to be clear: would something like the extension described in the Embedded notation and prefix layout section do more to address Servo's use case?

Or do you more broadly mean that we need all the things list in the Other changes section that followed that?

Or do you mean that there is something Servo needs that is not addressed by the items enumerated there?

@Manishearth
Copy link
Member

Manishearth commented Mar 17, 2016

@pnkfelix embedded notation works pretty well.

@golddranks
Copy link

golddranks commented Mar 17, 2016

@jFransham I don't quite understand what you're saying, but traits are basically interfaces. You code against an interface. If you mean by "concrete value" a field in the struct that implements a trait, how could you code generically against the interface, without the trait saying whether the value is mutable or immutable? You can't.

@eddyb
Copy link
Member

eddyb commented Mar 17, 2016

@golddranks The mutability depends on whether the value implementing the trait is in a mutable slot or not. This is already the case, e.g. if you write accessors, you can call setters that take &mut self only if the value is in a mutable slot.

@golddranks
Copy link

golddranks commented Mar 17, 2016

@eddyb Ah, I see. Pardon my ignorance.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 17, 2016

@petrochenkov

Any plans to incorporate mutability in this RFC?

I did not have any such plans. It's an interesting thought. I sort of wish we declared fields as mut as well, in which case this would be consistent. But we don't.

I think I would be more in favor if we also planned to add a mut keyword to ordinary fields and then to lint against assigning directly to a field (or a projection from a field) unless it is declared as mut. Put another way: you would be able to assign to x.f if (a) x is mutable, as today and (b) f is mutable. If (a) is violated, you get an error (as today). If (b) is violated, the code compiles, but you get a lint warning. Obviously the rules would still be mildly different then but if you followed "best practices" it would be consistent.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 17, 2016

@Manishearth

Note that as an inheritance system this is not useful for Servo's DOM. This

I think you and @pnkfelix hashed this out, but to be clear: as the RFC states, this RFC alone is not intended to solve Servo's "DOM problem", but it is a major building block towards doing so. The section on future work lays out the additional steps that I believe would be needed to make a truly ergonomic DOM implementation. If you think anything else is required, it'd be good to speak up. =)

@Manishearth
Copy link
Member

Manishearth commented Mar 17, 2016

Yeah, understood. I personally don't really feel Servo needs a better DOM solution at this stage (If it exists, sure, we'd use it, but I don't want to push for it). I'm happy with our current set of hacks, and the only (minor) improvement I'd like would be some layout-guarantees so that the transmutes aren't technically UB.

I was just pointing out that if the motivation behind this was partially due to use cases like Servo's DOM, it doesn't apply cleanly there.

Basically, Servo would need cheap upcasting. I think with this proposal you need to use trait objects to get that effect, if it is even possible.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 17, 2016

I don't have time to edit the draft now, but I've added a "planned edits" section to the main area.

@kbknapp
Copy link

kbknapp commented Mar 18, 2016

Huge 👍 I've wanted this for a while but there's no way I could have articulated it that well!

@alexreg
Copy link

alexreg commented Jan 21, 2018

No progress on this still...?

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jan 25, 2018

I am nominated this RFC to discuss in the @rust-lang/lang meeting. In particular, I'd like to decide how it fits in our overall priorities and what we ought to do with it.

I also owe some updates (well, perhaps on the dedicated repo) so discuss a few shortcomings of the RFC that I think still need to be addressed, but hopefully that'll come soon.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jan 26, 2018

@rfcbot fcp postpone

OK, after some discussion in the @rust-lang/lang meeting, it seemed clear that while we are still interested in a change like this, we don't have the bandwidth to push this through right now, so we're going to postpone the change.

I'm going to keep the repo open and also try to catch up on the conversation there and highlight some of the most important open questions though.

If anyone is up for it, I'd be interested in working closely with someone to keep the design going, so we can perhaps revisit it once things are calming down.

@rfcbot
Copy link

rfcbot commented Jan 26, 2018

Team member @nikomatsakis has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@kevincox
Copy link

kevincox commented Jan 28, 2018

If anyone is up for it, I'd be interested in working closely with someone to keep the design going, so we can perhaps revisit it once things are calming down.

I don't know exactly what this would entail but I have run into multiple use cases for this and would be glad to work on it as a new rust contributor.

@rfcbot
Copy link

rfcbot commented Jan 31, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

2 similar comments
@rfcbot
Copy link

rfcbot commented Jan 31, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link

rfcbot commented Jan 31, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jan 31, 2018

@kevincox can we maybe schedule some time to chat? reach out over gitter or IRC or e-mail maybe?

@rfcbot
Copy link

rfcbot commented Feb 10, 2018

The final comment period is now complete.

1 similar comment
@rfcbot
Copy link

rfcbot commented Feb 10, 2018

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Feb 14, 2018

Closing as postponed; activity should happen on the dedicated repo.

@aturon aturon closed this Feb 14, 2018
@kevincox
Copy link

kevincox commented Mar 6, 2018

I created https://internals.rust-lang.org/t/fields-in-traits/6933 to further discuss use cases. I started it off with some broad categories but would be interested in hearing more, and more specific use cases.

@Centril Centril added the postponed label Mar 7, 2018
@eddyb eddyb mentioned this pull request May 25, 2018
@mati865
Copy link

mati865 commented Jun 3, 2019

OK, after some discussion in the @rust-lang/lang meeting, it seemed clear that while we are still interested in a change like this, we don't have the bandwidth to push this through right now, so we're going to postpone the change.

Is it the right time to reconsider this?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

I don't think it really fits into this year's roadmap of "cleanups and finishing things"

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 3, 2019

IIRC @ubsan and @rpjohnst had an interesting discussion on discord about this problem which is not written down anywhere. Internals might be a good place to continue that.

@tiby312
Copy link

tiby312 commented Jan 19, 2020

I just wanted to say I think this is a great idea. I've been making a 2d graphics library in rust, and I usually have to result to unsafe code when I want to force a trait object function to return a reference to a member of itself.

trait HasPosition{
    fn pos(&self)->&[f32;2];
}

WIth this trait I can't guarentee that the reference returned from pos() is

  1. A reference to a member of the object.
  2. Is always the same offset into said object.
  3. Always returns the same value provided I have a read-only reference to the object.

Using the accessor methods in that you lose all these guarentees.

In order to get these guarentees, I have to make the trait unsafe, and add documentation that the implementor of HasPosition must uphold.

I want these guarentees so that I can figure out the vertex stride when setting the opengl vertex attributes. Not really a thing you do everyday, but still an issue for my unusual use case.

So this issue I'm having isn't a problem with code-reuse, but it is just the fact of losing guarantees when using accessor methods over straight up member access. I think giving the traits members where the implementer can map the trait member to one of their own members is a great idea and doesn't have the multiple inheritance problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tracker
Merge proposed
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.