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

Tracking issue for RFC 2300, "`Self` in type definitions" #49303

Closed
Centril opened this Issue Mar 23, 2018 · 16 comments

Comments

Projects
None yet
5 participants
@Centril
Contributor

Centril commented Mar 23, 2018

This is a tracking issue for the RFC "Self in type definitions" (rust-lang/rfcs#2300).

Steps:

Unresolved questions:

  • This syntax creates ambiguity if we ever permit types to be declared directly within impls (for example, as the value for an associated type). Do we ever want to support that, and if so, how should we resolve the ambiguity? A possible, interpretation and way to solve the ambiguity consistently is discussed in the rationale.
@csmoe

This comment has been minimized.

Member

csmoe commented Mar 26, 2018

I have tried to solve this, but the dfficulty of this is beyond my current rust-experience so I'd appreciate any mentorship.

@Centril

This comment has been minimized.

Contributor

Centril commented Aug 2, 2018

re-cc @rust-lang/compiler - could we get some mentoring instructions perhaps per @csmoe's request?

@alexreg

This comment has been minimized.

Contributor

alexreg commented Aug 14, 2018

Currently working on this...

bors added a commit that referenced this issue Aug 18, 2018

Auto merge of #53324 - alexreg:self_in_typedefs, r=eddyb
`Self` in type definitions (self_in_typedefs)

This implements the [`self_in_typedefs` feature](https://github.com/rust-lang/rfcs/blob/master/text/2300-self-in-typedefs.md) ([tracking issue 49303](#49303)).

r? @eddyb

CC @Centril
@alexreg

This comment has been minimized.

Contributor

alexreg commented Aug 18, 2018

You can tick off "implemented" now. :-)

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 11, 2018

@rfcbot merge

Stabilization report & proposal

Feature name: #![feature(self_in_typedefs)]
Version target: 1.32 (2019-01-18)

Originally accepted in rust-lang/rfcs#2300, I propose that we stabilize self_in_typedefs.
This has been in nightly since 2018-08-19 which is ~12 weeks ago.

The proposed change is not a blocker for anyone but it is a nice quality of life feature in some cases.
See the motivation of the RFC for reasons why we would want to do this.

What is being stabilized

Relevant artefacts:

The changes are:

  1. Self becomes a legal type in struct, enum, and union type definitions and refers to the type being defined. Self is permitted both in fields and in where clauses; For example:
enum List<T>
where
    // In where clauses, both on the side of types being constrained and on the bounds side.
    Self: PartialOrd<Self>
{
    Nil,
    Cons(T, Box<Self>) // In fields. The usual rules re. infinite types apply.
}

What is not being stabilized

Mentions of Self in trait, andtype aliases, and existential types are not being stabilized here.

Divergences from RFC

None

Unresolved questions

There is currently one unresolved question, it reads as:

This syntax creates ambiguity if we ever permit types to be declared directly within impls (for example, as the value for an associated type). Do we ever want to support that, and if so, how should we resolve the ambiguity? A possible, interpretation and way to solve the ambiguity consistently is discussed in the rationale.

At this point I believe that the discussion in the RFC is sufficient and that there is no problem should we ever want to pursue the extension aforementioned.

Thus, I suggest that we resolve the question as a footnote that may have some value in the future but does not impact us now.

@rfcbot

This comment has been minimized.

rfcbot commented Nov 11, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 11, 2018

I still feel that this brings some harm in terms of code readability/searchability and should be generally discouraged.

Additionally, interactions with #51994 are not yet implemented and value Self inside of a struct does not refer to the struct's constructor.

@alexreg

This comment has been minimized.

Contributor

alexreg commented Nov 11, 2018

We allow it pretty much everywhere else. I don’t see a difference here.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 11, 2018

@petrochenkov

I still feel that this brings some harm in terms of code readability/searchability and should be generally discouraged.

I think searchability and readability are not the same thing; Self is less good generally for searchability if used in impls or in type definitions; I don't think this is special to type definitions such that impls don't have that problem.

However; when it comes to readability, given that we chose to have an implicit head parameter to traits in the form of Self, we might as well make the most of it and apply it uniformly. For me, Self, whether in type definitions, or when used as -> Option<Self::Item>, or elsewhere, removes a mental burden of checking whether the type being mentioned is the same as in the head. If I can read a type definition and see Self then a cognitive load has been lifted.


Additionally, interactions with #51994 are not yet implemented and value Self inside of a struct does not refer to the struct's constructor.

What interactions? Can you elaborate (because I don't recall there being any interactions)? Also please elaborate on "value Self inside of a struct...".

Before stabilizing, we might want to extend the set of tests to include:

struct Foo;

impl Foo {
    fn bar() {
        struct Baz(u8, Option<Box<Self>>);
        let cons: Option<Box<Baz>> = None;
        let _ = Baz(1, cons); // Make sure that `Self` refers to the right thing
    }
}
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 11, 2018

What interactions?

Well, you need something contrived to get it, but something like

struct S([u8; Self]);

should not result in the "cannot find value Self in this scope" error.

Obviously, not critical.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 11, 2018

@petrochenkov Oh I see; so to make actual use of it you would need to write:

#![feature(self_struct_ctor)]
struct S([u8; weird(Self([]))]);
const fn weird(_: S) -> usize { 0 }
// This has a cycle I think
// Yeah...
// error[E0391]: cycle detected when const-evaluating + checking `S::0::{{constant}}`

(enum discriminants or rust-lang/rfcs#1806 might be more realistic use cases...)


I re-checked the RFC now and apparently I wrote:

The identifier Self is (now) allowed in type contexts in fields of structs, unions, and the variants of enums.

It explicitly says type contexts (and not value contexts), so going by the letter of the RFC, the implementation is correct and the error message should occur (tho it should be differently phrased, but I think we can improve diagnostics later for this pathological case...). I think we can probably revisit this later. :)

@alexreg

This comment has been minimized.

Contributor

alexreg commented Nov 30, 2018

Stabilization PR (for when FCP finishes, hopefully)

@rfcbot

This comment has been minimized.

rfcbot commented Nov 30, 2018

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

Centril added a commit to Centril/rust that referenced this issue Dec 1, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril

bors added a commit that referenced this issue Dec 3, 2018

Auto merge of #56366 - alexreg:stabilise-self_in_typedefs, r=Centril
Stabilize self_in_typedefs feature

[**Tracking Issue**](#49303)

r? @Centril

kennytm added a commit to kennytm/rust that referenced this issue Dec 3, 2018

Rollup merge of rust-lang#56366 - alexreg:stabilise-self_in_typedefs,…
… r=Centril

Stabilize self_in_typedefs feature

[**Tracking Issue**](rust-lang#49303)

r? @Centril
@rfcbot

This comment has been minimized.

rfcbot commented Dec 10, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril

This comment has been minimized.

Contributor

Centril commented Dec 10, 2018

An issue is filed in rust-lang-nursery/reference#473 for the documentation.
As such, there's nothing more left to do in this issue and therefore we can close it.

@Centril Centril closed this Dec 10, 2018

@alexreg

This comment has been minimized.

Contributor

alexreg commented Dec 10, 2018

@Centril I believe the reference is already up-to-date.

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