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: #[derive unfinished(..)] and #[unfinished] #2205

Closed
wants to merge 2 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 3, 2017

Rendered.

Allows the user to derive any trait for any type with #[derive_unfinished(Trait1, Trait2, ...)] by:

  • panicing via unimplemented in any method,
  • using ConstDefault for associated constants where implemented,
  • using defaults for associated types if such a default exists.

This desugars to a new #[unfinished] impl Trait for Type { .. } construct where the user may leave out any method they wish to not give an implementation for currently, as well as specify any associated type which does not have a default in the trait or any associated constant for which the type is not
ConstDefault.

The #[unfinished] construct in turn desugars into a full implementation of the trait for the type which uses unimplemented for methods and uses defaults where possible.

@Centril Centril changed the title Rfc/derive unfinished RFC: #[derive unfinished(..)] and #[unfinished] Nov 3, 2017
@Xion
Copy link

Xion commented Nov 3, 2017

This looks like an attempt to solve lack of tooling via a language feature. Editors & IDEs should be able to autogenerate a skeleton impl with unimplemented!() method bodies for any trait and type, similarly to how e.g. Java or C# IDEs can do so for class interfaces.

@Centril
Copy link
Contributor Author

Centril commented Nov 3, 2017

@Xion Discussed in alternatives ;)

I think @setharnold puts it well on #rust:

sarnold // centril: .. not to mention that the editor-generated framework
would work fine at a point in time, but not adapt as the trait is iterated

I think this approach is more uniform - and the tooling may not be there for every developer.

@durka
Copy link
Contributor

durka commented Nov 3, 2017

My thoughts:

  • This RFC is really about #[unfinished] impl and buries the lede. If #[unfinished] exists, then #[derive_unfinished] is a trivial syntax extension. This RFC has a lot of moving parts and it might be worthwhile to boil it down.

  • This isn't the only reason that you'd want to be able to query the items of a trait and similar things. For example, a facility for making forwarding impls of any trait for a newtype would want to do the same thing. Or an autogenerated RPC thing, etc. This makes me think that what we really want is a generalized way to have macros/plugins at this stage of the compiler, rather than having a huge RFC (you know this is going to have at least 100 comments) for each one.

  • As a minor comment, I'd want crates.io to reject uploads that contain #[unfinished].

  • I also agree that RLS could provide a similar function by generating an impl skeleton for you. Plus that helps you fill out the impl by showing you the items and their signatures. Ideally, you shouldn't have the unfinished impl around for so long that the trait definition changes.

So... overall I'm lukewarm on this. #[derive_unfinished] seems superfluous, while #[unfinished] seems useful, but maybe we actually want something generalized.

@Centril
Copy link
Contributor Author

Centril commented Nov 3, 2017

@durka Continuing on from #rust @ IRC:

  • Originally this started as the desire to have the equivalent of {-# LANGAUGE DeriveAnyClass #-} which is the equivalent GHC extension of #[derive_unfinished(..)]. The #[unfinished] bit was an idea that @aturon sparked from our conversation about #[derive_unfinished(..)].
  • I'll be working on an RFC for the equivalent of {-# LANGAUGE GeneralizedNewtypeDeriving #-}. A lot of the logic for this RFC will be useful for that one - especially wrt. generics and grammar of the attribute.
  • In addition, an RFC for {-# LANGAUGE StandaloneDeriving #-} might be useful.
  • I think a more generalized approach might be worth while in the end, but a more concise approach for special cases might be useful as a start.
  • The comment about crates.io rejecting #[unfinished] seems like a good change - but an upload should still be accepted it if the user writes #[allow(finished_unfinished)].
  • About RLS: I think it would be a good idea for RLS to do this as well - but you won't get the same terseness - and speaking for myself as a person with a bit of a short attention span, having a bunch of unfinished code around instead of something terse can be in the way... However, when you are iterating a trait and types in concurrently, having #[unfinished] impl around seems to me entirely feasible.
  • If everyone finds #[unfinished] to be the most useful part of this RFC, I can always write a new RFC that only includes that - #[derive_unfinished(..)] can always be added on top later on.

@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the RFC. label Nov 3, 2017
@est31
Copy link
Member

est31 commented Nov 6, 2017

👎 I don't want this RFC. Only because haskell has a feature doesn't mean that Rust has to get it, too. I think extending RLS to auto-output such impls would be the way forward. This is proposing to add a language feature which usually has a high barrier of entry.

but you won't get the same terseness - and speaking for myself as a person with a bit of a short attention span, having a bunch of unfinished code around instead of something terse can be in the way

I don't think that terseness during prototyping should ever be an issue. You should be seeing that there is something yet to implement! When I do development, I only do a quick glance when putting a working implementation into git, I could easily miss any unfinished directive. Also, having stubs already around makes implementing easier, as you will only have to fill in the implementation, instead of adding signatures. Stubs are no "clutter" as the RFC states!

Furthermore I wonder about the usefulness of turning off warnings about presence of unfinished. The RFC states that the feature is to allow for a "typecheck first, implement later" approach. For that goal, it would be sufficient if rust would issue a late stage (post monomorphisation) error if it encountered unfinished anywhere.

@Centril
Copy link
Contributor Author

Centril commented Nov 6, 2017

@est31

  • Nowhere does the RFC motivate itself by referring to Haskell - it only explains that Haskell is its inspiration and gives context for Haskell developers reading the RFC.
  • I disagree vehemently regarding terseness during prototyping - it is important! The longer a file is (including stubs) the more difficult it is to navigate through everything. When doing #[derive_unfinished(Trait)] you might have a few types and a few traits and it's not time for implementing them but rather to check if the traits and types work together wrt. lifetimes and such.
  • Raising an error was considered, however: you might want to run other tests and make sure everything else runs as expected even with unfinished.

@durka
Copy link
Contributor

durka commented Nov 6, 2017

I think there might be a small amount of confusion about warnings.

The RFC specifies a warning if you write #[unfinished] but you actually did finish the impl. And you can disable this warning if you want. Personally I don't care about this either way.

Where I want a warning, and potentially one that cannot be turned off (at least when uploading to crates.io) is if you use #[unfinished] at all and don't finish the impl, since it's supposed to just be a prototyping crutch. I think @est31 is talking about this second case as well though I'm not certain.

@scottmcm
Copy link
Member

scottmcm commented Nov 7, 2017

First of all, great job dealing with a ton of complicated cases in this, @Centril 🥇

I agree with @durka's "moving parts" comment, though. "Dealing with Generics", for example, is a huge part of the RFC, but is only needed for the derive part, so I think I'd prefer this separated.

@Centril
Copy link
Contributor Author

Centril commented Nov 7, 2017

@scottmcm Thanks <3. I think I agree too... So, the RFC should be split in two, one for derive_unfinished and one for unfinished. If you agree I'll do the change whenever I have some time =)

@Xion
Copy link

Xion commented Nov 7, 2017

Some random comments from me, of which some are I think mirroring est31's position.

I'll be working on an RFC for the equivalent of {-# LANGAUGE GeneralizedNewtypeDeriving #-}. A lot of the logic for this RFC will be useful for that one - especially wrt. generics and grammar of the attribute.

I'm a little skeptical that Rust even has -- or indeed, can have, given the lack of HKT -- a sufficient number of useful derivable traits that'd warrant such an addition. There are crates like newtype_derive right now that take care of basically all the standard trait derivation through macros or proc_macros already. To suggest GND, it'd require a strong case for language users & library authors to define a large number of their own, fairly general/generic traits that other would then need to derive for newtypes -- something that's not a given, considering that Rust's type system is notably less powerful than Haskell's.

In addition, an RFC for {-# LANGAUGE StandaloneDeriving #-} might be useful.

On the other hand, I'd love standalone deriving. It'd allow me, as a crate user, to locally correct some of the mistakes of crate authors without going through burdensome cycle of forking, altering crate code, changing my [dependencies], PR'ing, hoping for a merge, waiting for a new release, and then finally reverting back to a crates.io dep.

I disagree vehemently regarding terseness during prototyping - it is important! The longer a file is (including stubs) the more difficult it is to navigate through everything.

In most cases, I'd consider this is a useful hint that the module should be split into multiple files. Introducing the stubs early can be even thought as beneficial in this regard, as it highlights the issue earlier and allows to resolve it more easily, before the actual trait impl might have become more tightly coupled with the rest of the module (which makes the refactoring harder).

@Diggsey
Copy link
Contributor

Diggsey commented Nov 8, 2017

This is not just useful for prototyping, it's also useful for testing: often for component-level testing I use a dummy implementation of a trait in place of the real dependencies. The dummy implementation often only needs to implement one or two methods from the trait for any given set of tests.

To do this at the moment I end up giving all of my trait methods default implementations that panic. This is great for the tests, but it means that I don't get any errors if I add a new method and forget to implement it for one of the real implementations of the trait.

@Centril
Copy link
Contributor Author

Centril commented Nov 10, 2017

So; I think this RFC is a bit too ambitious for the impl period, so I'll close it for now =)

@Centril Centril closed this Nov 10, 2017
@Centril Centril mentioned this pull request Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants