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 traits #2309

Closed
wants to merge 1 commit into from
Closed
Changes from all 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
91 changes: 91 additions & 0 deletions text/0000-inherent-traits.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
- Feature Name: inherent_traits
- Start Date: 2018-01-21
- 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:

- 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.

- Frequently used types.

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.

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

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

```rust
pub struct Foo;

trait Bar {
fn bar(&self);
}

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

#[include(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.
Copy link
Member

Choose a reason for hiding this comment

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

Contemplating this for myself, what are the other options?

Some bad ones:

  • Inherit pub from the trait: gets into all the "what's pub, really" discussions
  • Put something on the methods: confusing to have pub there suddenly, doesn't work for trait methods with default impls
  • Put something on the trait: no, traits shouldn't ever care about this

A possibility: #[inherent(A)] puts pub(A) on them. So just #[inherent] is pub, but you can use #[inherent(crate)] (or maybe #[inherent(in foo::bar)]?) to limit it if helpful.

But really, in the same crate the trait import isn't a big deal, so that's probably not worth bothering with, and just "it's always pub" (as the RFC says) is probably the way to go 👍


The `impl` block may be empty, in which case the only methods defined on the type are those from any included traits.

The `include` attribute may include multiple traits.

The `include` attribute is not valid on `impl <trait> for <type>` style `impl` blocks.

# 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 seems a bit underspecified for traits with type parameters.

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

The `include` attribute in the above example makes the `impl` block equivalent to:

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

Choose a reason for hiding this comment

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

I'm unhappy with merging items from different trait impls together with each other and inherent impl items (syntactically) for reasons described in #2303 (comment).
impl Bar for Foo { ... } should still be a separate item, IMO, just marked as "inherent-like" somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petrochenkov I've read your comment a number of times and I can't really make any sense of it - this RFC does not propose allowing implementing traits via inherent methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. I see.
To be honest I read it a couple days before writing this comment and it looks like I misremembered everything.

There was a few previous iterations on this idea, some of them made methods from specially marked trait impls directly available to name resolution without having the trait in scope and without delegation, like this RFC proposes.
(Trait objects currently behave very similarly, see e.g. rust-lang/rust#41221)


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

Any questions regarding coherence, visibility or syntax can be resolved by comparing against this expansion, although the feature need not
be implemented as an expansion within the compiler.

# Drawbacks
[drawbacks]: #drawbacks

- Increased complexity of the language.

# Rationale and alternatives
[alternatives]: #alternatives
Copy link
Member

Choose a reason for hiding this comment

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

What about inverting the declaration location to something like

#[inherent]
impl Foo for Bar {
    ...
}

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this in the lang team meeting-- IMO placing the attribute on the impl "feels" better, but it doesn't seem like it would work with derived traits and generic imps (e.g. impl<T> ToString for T where T: Display { ... } -- how do I get an inherent to_string for u32?).

Copy link
Member

Choose a reason for hiding this comment

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

Ah good point.


- Do nothing: users may choose to workaround the issue by manually performing the expansion if required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative: Allow inherent impls as long as all functions are crate-private. Inherent methods are preferred over trait methods, but warned about. So if another crate adds a trait method, it does not conflict or change behaviour here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inherent methods are preferred over trait methods, but warned about.

I don't think they are warned about (see my playground link above)?

I think making the functions crate-private would undermine the primary use-cases of this RFC. The goal is to expose trait methods as inherent methods on a type, as part of the public API of a crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... one could allow public inherent methods and require importing them via use crate_name::impl::Type; or something similar


# Unresolved questions
[unresolved]: #unresolved-questions

- Syntax bike-shedding
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps #[delegate(Bar)] ?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me "delegation" has always been about making a trait impl for some type reuse the code from some other type's impl of the same trait, and the presence of delegation would be completely unobservable to client code. impl to another type. So "delegating to a trait", in a way that changes whether or not clients need an import, feels to me like a category error at best.

Though I'm certainly not a fan of #[include(Trait)]. That looks so similar to a C/C++ #include that I had to pause and think to figure out what the RFC was actually proposing.

Copy link
Contributor

@Ixrec Ixrec Jan 22, 2018

Choose a reason for hiding this comment

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

So, I assume the bar() method signature in the Reference-level Explanation is meant to appear in rustdoc. And presumably, you could replace the usage of the "inherent impl" feature with writing out those methods by hand, without clients being able to observe the change in any way.

If I'm interpreting that right, perhaps the "correct"/least misleading/most guessable-by-newbies syntax would be a macro rather than an attribute?

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

Incidentally, even if everyone else prefers an attribute, I'd still prefer that the attribute be placed inside the impl block rather than on top of it, simply because we're not modifying the impl block as a whole, we're adding to it.

Copy link
Contributor

@Centril Centril Jan 22, 2018

Choose a reason for hiding this comment

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

To me "delegation" has always been about making a trait impl for some type reuse the code from some other type's impl of the same trait

To me too =) But I thought it felt sufficiently similar in spirit that it might work?
I'm not particularly fond of #[include(Bar)] (but could certainly live with it..).

If I'm interpreting that right, perhaps the "correct"/least misleading/most guessable-by-newbies syntax would be a macro rather than an attribute?

That would have to be a very magic and special macro since they don't have access to the trait definition otherwise.

Copy link

@burdges burdges Jan 22, 2018

Choose a reason for hiding this comment

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

Why not impl Foo { #![include_trait(Trait)] } so inside the inherent impl block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@burdges I think those two (#[ outside and #![ inside) are interchangeable, so that's the same as this proposal (modulo the naming)

Copy link

Choose a reason for hiding this comment

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

Afaik, all inside attributes use #![..] while all outside attributes use #[..] but not all inside attributes can be used outside and visa versa, so I'm restated @Ixrec last proposal with a fresh coat of paint.