Allow inheritance between structs.

[nrc]

No subtyping, no interaction with traits. Partially addresses #9912.

[chris-morgan]

In the case of traits, it was decided that trait X: Y { } was to be preferred over trait X : Y { }. I believe that this should use ": " rather than " : " for consistency.

[chris-morgan]

(Comparing with how it's implemented for trait, putting the space in the string is probably not the optimal thing to do anyway. word_space(&mut s.s, ":") is probably better, though I haven't traced through the code to see what that actually does.)

[jdm]

Will syntax like Sub { y: 11, .. Sup::new(x) } work? It seems like it could be desirable not to have to list every parent type's fields in the initializer for DRY purposes.

[huonw]

I can't see any tests for

structA:B{ ... }fnfoo(_:&B){}fnbar(a:&A){foo(a)}

does this patch support that yet?

[nrc]

@jdm It does not - we still require all the fields in any .. expr in the initialiser (I think). This sounds like a good thing to add though. There are a few questions about whether we should allow only a direct super-struct, or any transitive super-struct. We might also want to allow multiple expressions, which would be complicated. We might want a more general mechanism which allows a .. expr to provide only some of the fields, and then the super-struct cases would 'come out in the wash'.

[nrc]

@huonw No, I explicitly avoided adding any subtyping or coercions here. I think we need to think a little more about the implications of that, but I foresee adding it in another PR.

[alexcrichton]

Should sub-structs have access to private fields in parent super structs? It seems the answer is "yes" from this implementation, but it may be worth considering this and testing it one way or another. I would mildly expect the answer to be no.

[bill-myers]

I kind of wonder if a syntax where you specify a field as "use foo: S" where the "use" keyword is syntax sugar that imports the fields in the struct namespace would be a better design, since that would give access to the base struct for free without having to introduce a "super" keyword, would allow "multiple inheritance" (this could be extended to allow importing single fields via use), and would be a simpler change to the language.

Trait inheriting structs can then be an orthogonal feature, where the first field of the struct is considered as being the "parent" structure, regardless of whether it was imported or not via "use" (but with the restriction that only parents that are public are considered parents if the trait impl is in another crate, so that changing the first field of a struct is not an API change when the field is private).

[emberian]

@bill-myers super is already a keyword for relative imports; use super::foo

[nikomatsakis]

I'd like to review

[nrc]

@nikomatsakis I was going to address the comments others have made here (in particular, adding debug-info and privacy checks). You might want to hold off till then for a proper review. In the mean time, any comments on the general idea would be appreciated.

[nikomatsakis]

On Wed, Feb 26, 2014 at 02:08:50PM -0800, Nick Cameron wrote:

@nikomatsakis I was going to address the comments others have made here (in particular, adding debug-info and privacy checks). You might want to hold off till then for a proper review. In the mean time, any comments on the general idea would be appreciated.

Will do.

[brson]

Is this design written up somewhere?

[nrc]

@brson it is the first part of http://smallcultfollowing.com/babysteps/blog/2013/10/24/single-inheritance/

[nrc]

@nikomatsakis - ready for review

I'm not sure how much in the way of tests are required for debug-info - I added a simple test, let me know if we need more.

[dobkeratops]

a possible point in favour of having single inheritance: fewer symbols to remember when accessing struct members, which is a bigger deal without code-completion. "which sub-member contains which named field?" .. "i want .name' ..

if a primitive tool just lists all struct members after pressing '.' , a codebase that has single-inheritance will have a smaller amount of false posatives in that list

[dobkeratops]

One more idea,
if not having single inheritance, would you consider allowing element access to 'shortcut' where names are unambiguous.
example

struct Foo { id:int, name:~str }
struct Bar { foo:Foo, x:int, y:int }
let b=Bar::new(); println!("id={:?}", b.id) // a.id is a shortcut for b.foo.id ... since its unambiguous...

Even if you dont want that, it would be nice to make an error message that does that lookup for you... "Bar has no member id... did you mean .foo.id"

Without this I'm going probably going to end up making a trait of accessors. trait IFoo { fn id()->int..} impl IFoo for Bar ...

I thought that traits inheriting structs would also let you use struct elements in default implementations, which would also be really helpful.

This is about ease of navigating code, ease of refactoring.. if you start moving fields between structures.. (which should be part of the common base ? .. you evolve that decision over time as your source grows..) .. you have less to go back and rewrite.

[nrc]

Sorry the commits are kind of messy - lots of rebasing going on.

[nrc]

@nikomatsakis r? I rebased everything and added a requirement for the virtual keyword.

Its all feature gated, so if we do something different for the DOM problem it shouldn't be too hard to rip out. But if we do go the virtual structs route, then this is the first step and it will rot quickly if we don't land it, I think.

[nikomatsakis]

@nick29581 ok, I read through it. Everything looks fairly reasonable. I think the big comment is that I'd rather see the set of struct fields collected in a pre-pass or sidetable rather than scraping and re-scraping them from the AST. If you fear bitrot, would you prefer to land and then address that?

[nrc]

Thanks for the review. I can make the changes now. I was worried about bit rot if we leave this until we are ready to implement virtual structs or post-1.0, another few days won't hurt.

[nikomatsakis]

@nick29581 ping me when updated

[nrc]

@nikomatsakis pushed an updated version. It doesn't quite use a side table for all the fields of a struct. I did move some of the field checking and collation into the collect phase and I build side tables for the super-struct of a struct and the declared fields of a struct. (I couldn't create a table for all fields here because some of the super-structs may not yet have been encountered, of course we could do another pass, but more on that later).

So after collect, we never scrape the AST, but we don't have a simple mapping from a struct to all its fields. I add a few methods to ty for getting all the fields for a struct (in ast and ty flavours, annoyingly), so this is only done in one place, eliminating the errors you worried about. If you like, we could cache these lists of all fields here. I believe that is easier than doing this pre-emptively either as a new pass or part of an existing one. I haven't done it because I'm not convinced it is a worthwhile optimisation for now, but I don't have a philosophical objection if you think otherwise.

[nrc]

@nikomatsakis updated and ready for re-review

[nrc]

SIgh, needs a rebase

[nrc]

Rebased.

@nikomatsakis (or anyone else) please could you re-r+?

[nrc]

Oh ffs, needs another rebase - looks like I was right about this bit-rotting quickly...

[nrc]

Rebased again. Please could someone r+?

[nrc]

AGAIN?! :-(

[nrc]

OK, let's try again. Can someone r+ please?

[nrc]

OK, I am mystified by that failure - it seems to be because of a missing feature gate, but the required #![feature(struct_inherit)] is in the file. Also it passes locally. Anyone any ideas?

[nrc]

Oh, wait, did it merge anyway? I am confused.

[huonw]

This is still open, so nope.

The problem is the check-fast driver; it creates a single huge crate from all the tests that don't have // ignore-fast. i.e. your files are a submodule, and so the features don't work. Just add that ignoring comment to the tests.

[huonw]

(If #13288 lands this cycle, then this just needs a retry, the check-fast driver won't exist and so ignore-fast won't do anything.)

[nrc]

Could this get a retry and/or another r+ please?

[nrc]

Oh, someone changed an error message. That is a dumb way to fail tests. Sigh.

Rebased/updated again, please to r+?

[nrc]

Rebased and fixed the failing test. Could someone r+ again please?

[emberian]

Wow this landed!