use Self keyword

[danieleades]

use the Self keyword throughout

• Self is currently used inconsistently throughout this codebase
• using Self is usually less verbose, and more readable than stating the type and generics redundantly
• it's clear that methods returning Self are 'constructors'
• refactoring the names of types is easier, as the Self references do not need to be updated
• Using Self simplifies proc macros, as it avoids having to write out #type_name #ty_generics in every quote! block
• there are a number of uses of fn function(self: FullyQualifiedSelf) that can be simplified to fn function(self)

---

as @daxpedda points out, you can use clippy::use_self to identify these (i've added it to my local cargo config file)

[target.'cfg(feature = "cargo-clippy")']
rustflags = [
    "-Aclippy::type_complexity",
    "-Wclippy::use_self",
]

[alice-i-cecile]

Can you explain why this is an improvement? This is a lot of churn, and I'm not sure this is more readable.

[daxpedda]

To make this a bit more automated, probably clippy::use_self should be used.

@alice-i-cecile according to Clippy:

Unnecessary repetition. Mixed use of Self and struct name feels inconsistent.

[danieleades]

Can you explain why this is an improvement? This is a lot of churn, and I'm not sure this is more readable.

i've updated the task description. it is a lot of churn

[alice-i-cecile]

Thanks for the description update. Can you add the lint as a warning level to the CI and contributing instructions? Follow the example set by https://github.com/bevyengine/bevy/search?p=1&q=type_complexity.

I'm still not in favor of this change, but we should see what others think.

[james7132]

This is primarily a stylistic change, but one I can get behind, provided we indeed apply it consistently. I cast my vote of confidence for this kind of change.

There is a lot of churn here, but given the context it's pretty easy to see it's, for the most part, just a bunch of renames, which shouldn't be too big of an issue bar possible merge conflicts.

[mockersf]

I find code that overuses Self like that harder to read. I think it's better to use the type name most of the time.

Also, I would like to avoid nursery / pedantic clippy lints that don't have a clear value to the Bevy user. All the advantages you quoted are to the Bevy developer, and feel opinionated.

[ghost]

My opinion

I don't mind using Self or the type name itself.

I think the real problem this PR tries to solve is that we should be consistent throughout and not that we have to always use Self or the type name.

The best thing to do would probably be to talk about particular cases and see what we like best for the common use cases. This will probably result in having a mixture of Self and the type name which is totally fine IMO. We just have to be consistent with it.

In the cases listed below I prefer using Self for constructors, but Type for the parameters. Usually the constructors are at the start of the impl block so it is easy to see what Self refers to. Since this is the case the other methods are further away from the start of the impl block which results in having to scroll up or jump to the Type that Self is referring to.

Cases

Constructors

Only Self

pub fn new() -> Self {
    Self { .. }
}

Only Type

pub fn new() -> Type {
    Type { .. }
}

Mixture

pub fn new() -> Self {
    Type { .. }
}

pub fn new() -> Type {
    Self { .. }
}

Parameters

Only Self

pub fn do_stuff(x: Self, y: Self) {
    // todo
}

Only Type

pub fn do_stuff(x: Type, y: Type) {
    // todo
}

Mixture

pub fn do_stuff(x: Type, y: Self) {
    // todo
}

pub fn do_stuff(x: Self, y: Type) {
    // todo
}

[alice-i-cecile]

I agree that consistency is valuable. In general, I prefer the explicit types wherever possible: I find they're much easier to read in PRs and large impl blocks. This is especially true when Self is used as a parameter, rather than a return type.

[ickk]

I'll throw my bike in this shed;

I like the idea of using Self as the return type in the function signature of constructors only. It makes it very easy to see "this method is a constructor".

In a world where we would only use the name new for constructors, this wouldn't be a big deal. However there are potentially lots of constructors, i.e. ::from_xyz, et c., in Bevy. So using Self as the return type helps me when quickly scanning source code, since it stands out as different from any other function due to syntax highlighting.

[danieleades]

I... don't really see a clear consensus here, but I'll make a couple of observations and then we can argue about those

• there isn't a clear consensus on Self vs Type in all cases, though it's agreed that consistency is generally its own reward
• Self in 'constructor' type methods is generally preferred
• consensus is maybe leaning slightly towards Type for other sorts of methods
• the current lack of consistency is not ideal

so putting aside sticking with the status quo, there are 3 options

a. use Self everywhere
b. use Type everywhere
c. use a mixture, according to an agreed style guide

Some notes on the above-

• option a. has the advantage that it can be linted (and autofixed) using clippy. That is not the case for either of the other two options
• option c. seems to be the best match for the 'consensus' in the discussion above, noting that a style guide would need to be agreed and documented, and enforcement will be a manual best-effort basis.
• options b. and c. will have some edge cases where generics, lifetimes, or fully-qualified paths will mean that using Type is significantly more verbose and/or repetitive than using Self.
  • perhaps the hypothetical style guide can capture judicious use of Self by exception
• options b. and c. represent significantly more effort than doing nothing (ie closing this ticket and stepping away from the keyboard). Option a. is negligible effort to introduce and maintain.

my personal two cents - it's hard to see that defining, introducing, and maintaining a Self vs Type style guide is worth the initial pain, the ongoing pain, and the general churn. If "Self everywhere" is not the preferred style (or at least if the benefits are not considered to be worth the trade-offs) then i'm in favour of doing nothing at all.

[alice-i-cecile]

If "Self everywhere" is not the preferred style (or at least if the benefits are not considered to be worth the trade-offs) then i'm in favour of doing nothing at all.

This is a great analysis and I couldn't have stated this conclusion better myself. Closing this out.