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

Add RFC for deprecating method call syntax on traits #2611

Closed
wants to merge 1 commit into from
Closed

Add RFC for deprecating method call syntax on traits #2611

wants to merge 1 commit into from

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Dec 11, 2018

@sinistersnare
Copy link

I am against this. A benefit of traits is the seamless usage of a trait by its type. This is a large change to the Rust language that will be removed by such a seemingly simple RFC.

- `Ord::clamp` was rejected completely because it caused similar breakage.
https://github.com/rust-lang/rust/pull/44438
- `failure` broke on 2018-12-11 due to newly introduced ambiguous trait calls:
https://github.com/rust-lang-nursery/failure/issues/280
Copy link

Choose a reason for hiding this comment

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

Iterator::flatten broke Itertools::flatten too.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Dec 11, 2018

@sinistersnare then I take it you would be in favor the alternative I wrote here?

@m-hilgendorf
Copy link

m-hilgendorf commented Dec 11, 2018

So traits that utilize builder patterns go from this:

my_impl.with_foo(foo)
       .with_bar(bar)
       .with_baz(baz);

to this:

    Trait::with_baz (Trait::with_bar (Trait::with_foo (my_impl, foo) bar, baz));

?

@sinistersnare
Copy link

As I understand it, Rust does not actually require SemVer in its crate ecosystem. So, even if it is a breaking change, it should not be a 'required' breaking change. Of course, adding a default method is a breaking change, so maybe it is important that we educate that. However, there needn't be anything changed in the Rust language.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Dec 11, 2018

@m-hilgendorf oof, yeah I suppose they would.

@m-hilgendorf
Copy link

@Xaeroxe I don't know how it would affect the compiler but is it possible to use type annotation for method calls instead? Something like:

my_impl.foo::<TraitA>();
my_impl.foo::<TraitB>();

@CryZe
Copy link

CryZe commented Dec 11, 2018

I'd suggest adding this as an automatic lightbulb action for the VSCode extension (and IntelliJ plugin) that automatically rewrites method call syntax to UFCS for you.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Dec 11, 2018

As some people have brought up on Reddit this would pretty much completely break the Iterator trait too, and that kind of makes this a non-starter. I'm going to leave this open for a bit so as to bring attention to the issue I'm trying to resolve, but I don't believe this RFC should be merged as is.

@distransient
Copy link

While I'm personally against this specific solution to the problem, I do think that there's a large problem worth solving here, and it's preventing api-breakages from being published to crate patch versions, including "providing new trait functions with default implementations, or adding new traits to an existing struct" as an api breakage. I have not personally seen any work regarding this but what would be incredible is if the rust toolchain could check the equality of the type-level api between two crates, and if this check was enforced on patch publishing on both the client and server side of cargo.

@Ixrec
Copy link
Contributor

Ixrec commented Dec 11, 2018

There's a lot to say here, and most important is that this is far too huge of a breaking change for too little benefit to be considered seriously even with epochs, but I think my main reaction is bafflement that trait method syntax is being singled out.

The examples in the RFC are just a subset of "expected inference breakage". You can get plenty of similar situations with other kinds of type inference that aren't unique to trait methods (which is why I say "little" benefit; we can't categorically eliminate this problem without going to even greater extremes and nuking type inference entirely). The whole reason "expected inference breakage" is a phrase that exists is because the Rust community already agreed that having type inference in the language and allowing trait/type authors to add defaulted methods/new impls in a minor version were valuable enough to offset the cost of dealing with the breakage that implies. As that RFC put it:

Principles of the policy
...

  • Minor changes should require at most minor amounts of work upon upgrade. For example, changes that may require occasional type annotations or use of UFCS to disambiguate are not automatically "major" changes. (But in such cases, one must evaluate how widespread these "minor" changes are).
  • In principle, it should be possible to produce a version of dependency code that will not break when upgrading other dependencies, or Rust itself, to a new minor revision. ...

But I do agree that we could do better at mitigating this sort of thing. There should probably be a clippy lint for preferring Trait::method(&foo); over foo.method();, if there isn't one already. And whatever semver checking tools are out there should reject potentially inference-breaking changes in patch versions, if they don't already.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Dec 11, 2018

@Ixrec

There's a lot to say here, and most important is that this is far too huge of a breaking change for too little benefit to be considered seriously even with epochs

Yeah after further consideration I agree, this might be the worst idea I've ever had.

bafflement that trait method syntax is being singled out.

In my experience it's a very common variety of this kind of breakage, but noted.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Dec 11, 2018

I'll go ahead and close this now as I think the RFC process served its purpose. To summarize:

  • Not happening
  • A more reasonable avenue might be in Rust tooling, either for clippy or crates.io.

@Xaeroxe Xaeroxe closed this Dec 11, 2018
@Xaeroxe Xaeroxe deleted the deprecate-method-traits branch December 11, 2018 22:04
@Nemo157
Copy link
Member

Nemo157 commented Dec 14, 2018

I haven't seen it mentioned here, but I recall seeing a proposal that code could be automatically re-written into UFCS form when generating a package (not an RFC, I think it was just a side-note on i.rl.o at some point). That would mean that any package uploaded to a registry would avoid the breakage from newly introduced trait methods, but would still retain the benefits of method call syntax while developing the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants