[RFC] Changing the way we handle addition/changes to the traits #415
Conversation
therealprof
requested review from
dylanmckay,
jcsoo and
a team
as code owners
therealprof
changed the title
|
I'm happy with this |
|
👍 |
|
Looks good |
There was a problem hiding this comment.
I'm definitely in favor of requiring real-world implementations and users of traits before accepting/stabiliziing them, but I am not convinced that removing unproven is the right thing to do.
There seem to be some clear drawbacks, which I allude to below, that should be added to the "Drawbacks" section, even if we decide to go ahead with this RFC as-is.
|
|
||
| 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. | ||
|
|
||
| If a trait does not yet provide the expected quality, a new version can easily added under the new rules explained below. |
There was a problem hiding this comment.
Does "new version" here mean something like digital::v2 or a breaking change replacing the old one?
There was a problem hiding this comment.
Can be anything the group decides is the right thing.
|
|
||
| ## 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. |
There was a problem hiding this comment.
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 thatunprovenis 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
unprovenflag 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).
There was a problem hiding this comment.
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.
I'm a bit wary of adding new text to the text because this will invalidate all of the reviews. This has been sitting for quite a while in |
That's bad. The process shouldn't stand in the way of a good RFC like that. We should discuss turning this off and using a list of checkboxes instead (or even full-on rfcbot, but that's probably not worth it). |
Well, one or the other would do. ;) No objections to adding check boxes, except that it's quite tedious to do manually for the whole WG and I have no idea how to automate that. |
|
Looks like we have consensus (and we've already landed the docs changes in |
Sure. However I'm failing to distil any real drawbacks from the discussion above; I should mention that we already have people following the new process without problems, cf. rust-embedded/embedded-hal#212 or rust-embedded/embedded-hal#204 or rust-embedded/embedded-hal#191. I could add the mentioned full-blown RFC process as in the main Rust teams as alternative if it makes people happy. |
|
Yeah fair enough, I'm okay as it stands and we can PR changes or additions later if required. bors r+ |
|
Build succeeded: |
Rendered