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: Allow re-exporting associated items with use #1150

Closed
wants to merge 3 commits into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jun 3, 2015

Extend use to work in trait definitions and impl blocks for re-exporting associated
items under different names. This provides a clean way to rename items harmlessly, and
provides a more ergonomic way to provide the same functionality under different names
when necessary to satisfy an API.

This enables

trait Iterator {
    fn len_hint(&self) -> (usize, Option<usize>) {
        (0, None)
    }

    #[deprecated("renamed to len_hint")]
    use Self::len_hint as size_hint;
}

Rendered

@huonw huonw added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jun 3, 2015
@Gankra
Copy link
Contributor Author

Gankra commented Jun 3, 2015

Added an example.

@seanmonstar
Copy link
Contributor

I greatly prefer the alternative, where one could do fn foo = Self::bar or whatever.

@alexcrichton
Copy link
Member

I would personally find this somewhat surprising in that an attribute is now greatly affecting name lookup and method resolution, where attributes normally don't have much effect on these sorts of phases of the compiler. I would also probably prefer a more first-class fn foo(...) = bar or something like that, but I also don't personally feel so motivated to add this at this time.

to support some kind of "currying" like `type` does with `type<T> = Foo<Concrete, T>`?)

The RFC author would like to have a tool for this in the short-term, as there are
several outstanding rename requests.
Copy link
Member

Choose a reason for hiding this comment

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

I only know of 2:

  • size_hint -> len_hint
  • ExactSizeIterator -> ExactLenIterator

What are the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just off the top of my head

Copy link
Member

Choose a reason for hiding this comment

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

Some of those (I don't know about the rest) are Unstable though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only tail/init are unstable, and people are proposing renaming other methods in that RFC (first/last)

@codyps
Copy link

codyps commented Jun 4, 2015

I'd be also be in favor of generalizing use to work for this case: it is what we would already use (no pun intended) in other cases for renaming things. Is there something specific about how methods (vs functions and other names) are treated that makes expanding use more problematic?

@bluss
Copy link
Member

bluss commented Jun 4, 2015

@jmesmon The old name needs to be deprecated and the new name not, and rust must not allow implementing both method names. If that works, then it's fine.

@bluss
Copy link
Member

bluss commented Jun 4, 2015

The attribute acts like a rename and deprecation of the old value. Deprecations already use attributes, so that part is fine. The novel thing is just redirecting an old name, specified in the rename, to the new method.

@arthurprs
Copy link

I'm strongly in favor of this as it allows easier decisions ongoing.

But shouldn't this (optionally) go into the deprecated attribute itself?

@codyps
Copy link

codyps commented Jun 5, 2015

@bluss I suppose I'd expect one to have a non-deprecated implementation (of, in this case, the method) and a use to add the old, deprecated naming that was marked as deprecated. I suppose this also means we'd need to allow names created via use to be deprecated (if that isn't already allowed).

@llogiq
Copy link
Contributor

llogiq commented Jun 5, 2015

Currently, this could be done with

#[deprecated(since = "1.2.3")]
fn deprecated_method(...) { replacement_method(...) }

I don't think that situation is so bad that we need a new language feature to handle it, but then I'm not the one maintaining the language. 😉

@bluss
Copy link
Member

bluss commented Jun 5, 2015

@llogiq That is not enough to solve size_hint renaming in a satisfactory way.

@CloudiDust
Copy link
Contributor

I am for extending use (or fn, to a lesser extent). AFAIK, no renaming in Rust automatically deprecates the old name currently. And I am of the opinion that it should also be the case here. (Though an accompanying deprecation is usually wanted.) Then if all we do is a simple renaming, we should extend the existing language construct use, instead of introducing a new attribute.

@arthurprs, IMHO, extending deprecated would put the new name in the attribute and it will be harder to notice. Also the syntax may become weird if multiple renamings happen. A renamed attribute attached to the new name is clearer.

@Gankra Gankra changed the title RFC: rename attribute RFC: Allow re-exporting associated items with use Jul 1, 2015
@Gankra
Copy link
Contributor Author

Gankra commented Jul 1, 2015

I've completely rewritten the RFC to just use use per feedback.

@petrochenkov
Copy link
Contributor

Hm, why do type aliases even exist if they just duplicate the functionality of use?

use B as A;
type A = B;

Well, because type can do smarter things than use, like:

type S<T> = SU<T, DefaultU>;

Can the same logic be applied to function aliases? Do we ever need something like:

fn f<T> = g<T, DefaultU>;
// or
fn h<'a> = k<'a, 'a>;

?
If yes, then a specialized tool for function aliases would be a better solution than use. (But it can, of course, coexist with use, like type does.)

@arthurprs
Copy link

This is good @gankro 👍

@nrc
Copy link
Member

nrc commented Jul 9, 2015

-1 Why not just leave the deprecated method as just a call to the new method? This seems like a lot of added complexity (especially when reading code) for not very much gain. If this needs a solution (and I'm not sure it does), then it seems it should be done with tooling, rather than with a language feature.

@bluss
Copy link
Member

bluss commented Jul 9, 2015

@nrc: We need this for renaming/deprecating of methods that you usually chain / delegate off. The example that sparked this RFC was Iterator::size_hint.

@eddyb
Copy link
Member

eddyb commented Jul 9, 2015

@nrc that doesn't work. However, a simple attribute for renaming a method would be enough. It would just work (and you should keep it unstable while designing a better solution).

If @gankro wants to go the use route... It might be possible to get it somewhere. Maybe it could be useful for other things.

But it's a lot more specification and implementation work than this RFC assumes:
You have to make associated items of traits importable and exportable without breaking backwards compatibility (keep in mind use std::clone::Clone::*; compiles right now and imports nothing).
You have to allow non-re-export uses of use inside trait definitions.

Finally, you have to define re-export semantics in trait definitions that happen to also allow the kind of renaming desired here, which requires traits not only to expose those items, but also catch their implementations (only when the re-export is same-trait).

What would re-exporting something that isn't an associated item do? Just behave as if the trait was a module, and ignore them on method lookup or in <T as Trait>::associated paths?

In any case, 👎 for this RFC as it is written right now, it's missing a lot of details and it would not be prudent to accept it in this state. That said, I'm warming up to this use of use, as long as all ramifications are investigated.

@nrc
Copy link
Member

nrc commented Jul 9, 2015

"doesn't work" seems to mean causes some loss of efficiency and possible confusion for implementers. The trade off for size_hint in particular seems to be - having a sub-optimal name or some degree of performance loss (and implementer confusion), satisfying that trade-off doesn't seem worth extra language-level complexity.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 9, 2015

Am I right in my interpretation that everyone who is saying "that doesn't work" in response to @llogiq and @nrc is basing their statement on a (hypothetical) requirement that when deprecating an API due to a trivial rename, one wants to enforce that old_name and new_name are in sync?

NOTE: I used the word "trivial" not to make the problem sound easy, but rather to stress the fact that connecting up such synchronization in that specific case (where it is just a rename, no parameter reordering or conversions) seems to be tightly wound up with the feature being added here.

It seems to me like there are other ways, perhaps lint-based, to enforce such synchronization that, IMO, would have a prayer of generalizing to more complex scenarios.

Update: Off the top of my head, here's one idea: If the deprecated trait method can be expressed in terms of a default method that calls the new one in some manner, then we add an attribute to the deprecated method in the trait that says "warn on override". Where would that go wrong ? Implementers would be told they need to now implement the new method (if they have not already), and they would be told not to provide their own impl of the defaulted one anymore.

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC is now entering final comment period.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jul 9, 2015
@bluss
Copy link
Member

bluss commented Jul 9, 2015

@nrc: Ah but correcting a slightly worse name doesn't warrant the trouble that an imperfect rename involves. I think we need “perfect” name deprecations to even consider deprecations just because of these kinds of small blemishes in the varnish.

What I think of renaming size_hint specifically is that we shouldn't waste time renaming things that are so out of the way and are neither a user learning issue nor a user misunderstanding issue.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 9, 2015

@pnkfelix Yes, this RFC is largely trying to address the problem of keeping old and new names in sync, particularly when previous defaults prevent you from just building a default cycle or redirection.

@liigo
Copy link
Contributor

liigo commented Jul 10, 2015 via email

@nikomatsakis
Copy link
Contributor

Using use feels pretty inconsistent to me. If nothing else, this feels like it ought to be a pub use --- but in general I feel like use is used to bring things into scope. It seems very reasonable to me that one might want to allow use in traits/impls just for that purpose, as you can with blocks.

This comment is not meant as an endorsement of the underlying functionality. I have to think over how much work and complexity it is (as @eddyb suggests).

@huonw
Copy link
Member

huonw commented Jul 13, 2015

I like this functionality in theory. But it is relatively complicated and only serves... one use-case (size_hint) now, and the circumstances where it is needed seem relatively rare? Are there others that people have in mind?

@arthurprs
Copy link

Not really sure but we could generalize it to support renaming traits and structures as well.

@eddyb
Copy link
Member

eddyb commented Jul 14, 2015

@arthurprs But that's already possible!
Or do you mean deprecating a pub use while keeping the original item undeprecated?
In which case, if it doesn't work already, it's an order of magnitude less work (and I wouldn't even open a RFC for that, call it a bug fix).

@Gankra
Copy link
Contributor Author

Gankra commented Jul 14, 2015

I have definitely deprecated re-exports of modules and types in std before.

@arthurprs
Copy link

Ya, I'm felling stupid right now, sorry.

@huonw
Copy link
Member

huonw commented Jul 14, 2015

@gankro it doesn't work.

// deprecated.rs
#![crate_type = "lib"]
#![staged_api]
#![feature(staged_api)]
#![stable(feature = "x", since = "1.0.0")]

#[stable(feature="x", since="1.0.0")]
pub mod foo {
    #[stable(feature="x", since="1.0.0")]
    pub fn f() {}
}

#[deprecated(since="1.1.0")]
#[stable(feature="x", since="1.0.0")]
pub use foo as bar;
extern crate deprecated;

fn main() {
    deprecated::foo::f();
    deprecated::bar::f();
}

All compiles without a peep.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 14, 2015

Hmm... maybe I hallucinated that...

@Gankra
Copy link
Contributor Author

Gankra commented Jul 20, 2015

This proposal has enough issues that I'm just going to close it. I think the right idea is in here somewhere, but I'm not the one to find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.