Skip to content

[RFC] Changing the way we handle addition/changes to the traits #415

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 2, 2020
Merged

[RFC] Changing the way we handle addition/changes to the traits #415

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46c2b60
Create xxxx-revising-unproven.md
therealprof Jan 14, 2020
7911a21
Update and rename xxxx-revising-unproven.md to 0415-revising-unproven.md
therealprof Jan 14, 2020
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
56 changes: 56 additions & 0 deletions rfcs/0415-revising-unproven.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
- Feature Name: Changing the way we handle addition/changes to the embedded-hal traits
- Start Date: 2019-11-12
- RFC PR: https://github.com/rust-embedded/wg/pull/415/
- Rust Issue: (leave this empty)

# Summary
[summary]: #summary

This proposal proposes to change the way how we make additions and changes to the `embedded-hal` traits in order to break the radio silence.

# Motivation
[motivation]: #motivation

Our current approach to changes is to open an RFC issue/PR, propose a trait, discuss it extensively and then maybe add it under the `unproven` feature flag -- or often not. In essence this means that all proposals are somewhat theoretical, reluctantly implemented and the `unproven` feature gates usually never get removed due to a lack of motivation to do that extra work which also leads to everyone uncoditionally enabling the `unproven` feature.

This is not very efficient for the community and stifles implementation and innovation.

This RFC attempts to change from a theoretical approach to a more real life approach by favouring working implementations over dry discussion.

# Detailed design
[design]: #detailed-design

This RFC suggests a more progressive approach by changing the way the crate works and the way we accept new additions and changes.

## Disabling the problematic `unproven` feature

This crate should only contain proven traits, hence all existing traits should be automatically considered proven (also to not break compatibility) and thus all feature gates removed and the feature itself turned into a no-op.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is difficult to experiment with more experimental traits if the unproven feature is removed – if we require 2 independent impls, having an experimental version of the trait available makes writing and using them much easier.

Of course, the trait can still be published in some sort of thingy-trait crate, but that comes with higher overhead, reduced discoverability, and makes the ecosystem harder to navigate due to the many crates. I believe it is important to encourage experimentation here, since that is what experimental traits need to stop being experimental, so maybe reconsider doing this.

Instead, we could:

  • Make use of #[doc(cfg(unproven)))] to mark unproven APIs as such in documentation
  • Rename the feature to unstable, since Rust developers already know what that means and are (hopefully) less eager to enable it (only one sentence in the API docs mentions that unproven is the same as "unstable" and means that an API is semver-exempt)
  • Open tracking issues for every unstable API (which can be a trait + type definitions used by it), and document unresolved questions and the necessary steps to stabilize an API – clearly documenting the situation also has the side-effect of making it easier for people to contribute

Basically, adopt the things that have worked well for Rust itself. As I've mentioned before, this has also been done for async-std and seems to be working well there.

I know I have talked about this before, but I think it should at least be considered, and the reasons for doing it or not doing it written down.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a viable alternative to separate things by Rust's own stable/nightly then? Keep things simple for everyone and anything that's "unproven" will only be avilable for nightly builds by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's doable, yes. Although it would put another barrier in the way when someone wants to experiment with some unstable embedded-hal trait (install nightly + recompile everything). We should still make these opt-in via a Cargo feature, since some people are on nightly by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously the unproven flag just caused development to grind to a halt which is the main motivation to get rid of it.

I do not think it is unreasonable to do a PR to implement the traits and then either do two impls yourself or convince other people to contribute another one if two are too onerous to achieve. Implementing something against a PR branch is easy to do, I have to do that all the time to test PACs or HALs pr foundational crates so that shouldn't be a big hurdle. We can even provide some guidance on how to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implementing something against a PR branch is easy to do, I have to do that all the time to test PACs or HALs pr foundational crates so that shouldn't be a big hurdle.

I understand it's not a big hurdle for people already neck-deep in the WG, but it still has higher overhead than enabling a feature gate, and reduces discoverability of the ongoing work.

Obviously the unproven flag just caused development to grind to a halt which is the main motivation to get rid of it.

I guess what I'm worried about here is that a trait will get added just because two impls have been provided, but then at some point down the road another impl is attempted and fails because something has been overlooked. This is not an issue when changing the trait and issuing a breaking change isn't too big of a deal, but since we're working towards a 1.0 ecosystem this becomes a bigger issue. While we can add v2/v3/vN of any traits when issues come up, that seems very undesirable.

If the main motivation is making sure that development doesn't grind to a halt as soon as an unproven trait is added, I have also suggested other solutions that would likely accomplish this above.

An important job of an RFC is to take alternatives into account, and explain why the solution it proposes is the optimal one. I'm asking for this because it seems very unintuitive to me to do the opposite of what has worked well in the Rust project (insta-stabilizing the unproven traits instead of declaring them unstable API surface and iterating on them until their drawbacks have been determined and eliminated or deemed acceptable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm asking for this because it seems very unintuitive to me to do the opposite of what has worked well in the Rust project (insta-stabilizing the unproven traits instead of declaring them unstable API surface and iterating on them until their drawbacks have been determined and eliminated or deemed acceptable).

I don't see any precedence for what we're doing here anywhere in the Rust world. I would agree if we were talking about implementations but we're talking about traits here. Calling it unstable instead of unproven does not provide anything.


If a trait does not yet provide the expected quality, a new version can easily added under the new rules explained below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does "new version" here mean something like digital::v2 or a breaking change replacing the old one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be anything the group decides is the right thing.


## Accepting new additions

Instead of discussing a trait, adding it and hoping for implementations, all proposed changes should be required to demonstrate their usefulness by pointing to at least **two** independent and different implementations and at least one example utilizing the implementations. The implementations can live in separate branches or PRs of two or more repositories, are meant to demonstrate that the proposed traits can be implemented for arguably different hardware and are usable.

Based on these requirements a more meaningful and interactive discussion can take place, eventually resulting in acceptance of the aditions with working implementations already in hand.

# How We Teach This
[how-we-teach-this]: #how-we-teach-this

The new process would be documented accordingly so interested parties can easily follow it.

# Drawbacks
[drawbacks]: #drawbacks

None known.

# Alternatives
[alternatives]: #alternatives

Don't implement this RFC.

# Unresolved questions
[unresolved]: #unresolved-questions

* How many implementations do we require? Are two enough?
* Do we want to make a sufficient seperation of the mandatory implementations a hard requirement or a soft one? And if it's a hard requirement what would be the metrics for it?