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

RFC: inherent trait implementation #2375

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions text/0000-inherent-trait-impl.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
- Feature Name: inherent-trait-impl
- Start Date: 2018-03-27
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary
[summary]: #summary

Provides a mechanism to declare "inherent traits" for a type defined in the same crate. Methods on these traits are callable on instances of the specified type without needing to import the trait.

# Motivation
[motivation]: #motivation

There are two similar cases where this is valuable:

- Frequently used traits.

Sometimes getting the right abstractions require breaking up a type's implementation into many traits, with only a few methods per
trait. Every use of such a type results in a large number of imports to ensure the correct traits are in scope. If such a type is used
frequently, then this burden quickly becomes a pain point for users of the API,
especially if users do not care about writing generic code over traits.

- Mapping object-oriented APIs.

When mapping these APIs to rust, base classes are usually mapped to traits: methods on those base classes will need to be callable on any
derived type. This is sub-optimal because to use a class method a user must now know which class in the hierachy defined that
method, so that they can import and use the corresponding trait. This knowledge is not required when using the same API from an
object-oriented language.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

The feature is implemented using a new attribute which can be applied to trait
`impl` blocks:

```rust
pub trait Bar {
fn bar(&self);
}

pub struct Foo;

#[inherent]
impl Bar for Foo {
fn bar(&self) { println!("foo::bar"); }
}

impl Foo {
fn foo(&self) { println!("foo::foo"); }
}
```

The method `bar` is now callable on any instance of `Foo`,
regardless of whether the `Bar` trait is currently in scope,
or even whether the `Bar` trait is publically visible. In other words if `Bar`
is defined in one crate and `Foo` in another, the user of `Foo` will be
able to explicitly depend only on the crate which defines `Foo` and still use
the inherent trait's methods.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

Copy link
Member

Choose a reason for hiding this comment

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

This should be fleshed out, including:

  • What impl blocks it can be applied to (can the trait be foreign?)
  • Whether it works with #[fundamental]
  • Does it error if you have an inherent method with the same name?

Copy link

Choose a reason for hiding this comment

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

I'd think it should error if you have an inherent method with the same name, like if you had two inherent methods with the same name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What impl blocks it can be applied to (can the trait be foreign?)

Yes, it can. One of the main use-cases for inherent trait implementations is implementation of crates defined in external crates.

Whether it works with #[fundamental]

I am not sure why it should not, though I can't say I fully understand #[fundamental] semantics.

Does it error if you have an inherent method with the same name?

Initially I though it should error, but as I wrote below probably it may be better to issue a warning and shadow inherent trait methods by true inherent methods.

I will try to update the text based on your review! Thanks!

The `inherent` attribute in the above example makes the `impl` block equivalent to:
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved

```rust
impl Foo {
#[inline]
pub fn bar(&self) { <Self as Bar>::bar(self); }

fn foo(&self) { println!("foo::foo"); }
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
}
```

Any questions regarding coherence, visibility or syntax can be resolved by
comparing against this expansion, although the feature need not be implemented
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be explicitly specced out, especially since this expansion actually introduces an ambiguity in method calls.

as an expansion within the compiler.

# Drawbacks
[drawbacks]: #drawbacks

- Increased complexity of the language.
- Hides use of traits from users.
Copy link
Member

Choose a reason for hiding this comment

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

If this works the way I think it does, this actually changes our trait stability story, and that's a major drawback.

Currently, adding a defaulted method to a trait is not a major breaking change, at worst it will cause ambiguity errors, so it's considered minor.

Now, a dependency can add a method to a trait you #[inherent] impl, which can clash with a method of your own, causing your build to fail in a way that requires you to change your API to fix. We're largely okay with builds failing due to new ambiguities (clashing method names across traits, adding something to a module that's glob imported, etc) and such things are categorized as minor, which basically means it's fine to do as long as the fallout isn't too much. With this RFC, adding a defaulted method has the potential to break a library user in a way that requires them to rename a method in their public API, causing a breaking change for them.

This should be explored and addressed in this RFC, and as a bare minimum should be called out in this section.

(One "fix" is to only allow local traits)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another fix is to list out the method names that are inherent, although that becomes quite a burden for e.g. iterator methods.

Copy link

Choose a reason for hiding this comment

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

I'm unsure if the existing situation should be classified as so minor because you interact with many types almost exclusively through their traits. As an example, there are definitely traits with a new() method that defies the convention of being inherent new(), so those traits adding new() created exactly the same breakage you describe here.

As a fix, I'd suggest #[inherent] being a method attribute for items inside an impl, so

impl TraitWIthBadMethodNames for MyType {
    fn new() -> Self { .. }  // Not inherent

    #[inherent]
    fn new_plus() -> Self;  // Inherent but default body used

    #[inherent]
    fn foo() { .. }  // Inherent with body supplied here
}

And #[inherent] applied to the impl is equivalent to it being applied to all methods and associated types and constants.

Copy link
Member

@Manishearth Manishearth Mar 28, 2019

Choose a reason for hiding this comment

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

As an example, there are definitely traits with a new() method that defies the convention of being inherent new(), so those traits adding new() created exactly the same breakage you describe here.

Yes, but this breakage is easily fixed by using UFCS. This is not true for breakage caused in the world of #[inherent].

The "minor" terminology comes from the API evolution RFC, such changes may cause crates to stop compiling, however:

  • This can be fixed with a trivial change local to the crate
  • The fix does not break upstream crates

Without this notion of "minor" being allowed, crates wouldn't be able to add anything new (types, traits, functions, or methods) without it being considered a breaking change.

Furthermore, the API evolution RFC mentions in the trait method case that you should check to ensure the fallout isn't too great; which is where your new() example falls short: a trait adding a new() method would probably have lots of fallout.

In other words, when I use the term "minor" here, I'm using a precisely defined term from another RFC. Whether or not it is actually "minor" is irrelevant, I'm talking about what we do and don't consider breaking, which we have specced in terms of this major/minor categorization.

Copy link
Contributor Author

@newpavlov newpavlov Apr 1, 2019

Choose a reason for hiding this comment

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

Will it be possible to implement shadowing of inherent trait methods by true inherent methods? Compiler will warn on clashing inherent names while building crate which uses #[inherent], but will use true inherent method by default and for trait method you will have to use explicit Trait::foo(value). This way we will avoid code breakage on "minor" upstream changes.

Though I think in practice such collisions should be extremely rare.

Copy link
Member

Choose a reason for hiding this comment

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

Will it be possible to implement shadowing of inherent trait methods by true inherent methods?

I think that would be the ideal fix here: an inherent trait's methods get shadowed by the type's own methods. They could produce a warning lint when compiling the crate itself, so that people notice, and then cap-lints will suppress that when compiling a dependency.


joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
# Rationale and alternatives
[alternatives]: #alternatives

- Define inherent traits either on a type `T` or on an `impl T { .. }` block.
- Implement as part of the delegation RFC.
- Do nothing: users may choose to workaround the issue by manually performing the expansion if required.

The most viable alternative is delegation proposal, although arguably inherent
traits and delegation solve different problems with the similar end result.
The former allows to use trait methods without importing traits and the latter
to delegate methods to the selected field. Nethertheless delegation RFC and
this RFC can be composable with each other:

```Rust
struct Foo1;
struct Foo2;

#[inherent]
impl T1 for Foo1 {
fn a() {}
}

#[inherent]
impl T2 for Foo2 {
fn b() {}
}

struct Bar {
f1: Foo1,
f2: Foo2,
}

impl Bar {
// all methods from `T1` will be delegated as well
// though `T1` will not be implemented for `Bar`
delegate * to f1;
}

// method `b` will be accessable on `Bar` without importing `T2`
#[inherent]
impl T2 for Bar {
delegate * to f2;
}
```

# Prior art
[prior-art]: #prior-art

- https://github.com/rust-lang/rfcs/pull/2309 (previous closed PR)
- https://github.com/rust-lang/rfcs/issues/1880
- https://github.com/rust-lang/rfcs/issues/1971

# Unresolved questions
[unresolved]: #unresolved-questions

joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
- Do we want to introduce a new keword instead of using `#[inherent]`? In other
words do we want to write `inherent impl A for B { .. }`?