Stability annotations on generic parameters (take 2) #72314
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
estebank
approved these changes
May 20, 2020
There was a problem hiding this comment.
2020-05-18T03:30:47.9766891Z tidy error: /checkout/src/test/ui/stability-attribute/generics-default-stability.rs:35: line longer than 100 chars
2020-05-18T03:30:47.9768337Z tidy error: /checkout/src/test/ui/stability-attribute/generics-default-stability.rs:36: line longer than 100 chars
The code changes look reasonable on my end. Depending on merge order, this will conflict with #71481 and cause one of the two to need a rebase.
CC @petrochenkov who might have extra thoughts on the approach.
5 tasks
Avi-D-coder
changed the title
TimDiekmann
reviewed
May 21, 2020
|
NB: It's now possible to convert a pull request to a draft, so you don't have to mess around with the WIP-prefix: |
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
This is a followup to @varkor's #65083 (comment)
This is insufficient, we also must prevent reliance on the default type (see bellow). These are the tests that should produce a error, but don't yet. let _ = Struct1 { field: 1 }; // ERROR use of unstable library feature 'unstable_default'
let _: Struct1 = Struct1 { field: 1 }; // ERROR use of unstable library feature 'unstable_default'
let _: usize = STRUCT1.field; // ERROR use of unstable library feature 'unstable_default'
let _ = STRUCT1.field + 1; // ERROR use of unstable library feature 'unstable_default' |
Why is this? As long as, to the user, it's completely opaque (i.e. as if the default was hard-coded), then it's okay for the user to rely on the explicit type, no? |
|
|
|
My understanding is that we're never going to change the default type: we want to experiment with allowing users to change the type. We can't prevent people relying on the specific type of a generic parameter. If users should not rely on a concrete type, this should be hidden by the API. It would be a breaking change in general to change the default type, stable or not. |
|
What if we want to remove the parameter after trying it? |
|
I imagine that the situation is like this: we have a type Is there a situation you can think of where this won't be the case? |
|
That's exactly the use case why we want this feature. |
|
@varkor Your right this does seem to be sufficient for the allocators use case. |
|
I think the same proviso applies for anything regarding stability: it can be easy to accidentally make something stable. I think we have to rely on reviewers to catch these sorts of issues, though: I'm not sure how we could feasibly prevent it from the compiler's perspective. |
Is it possible in an easy way if the unstable type is used as public field? Anyway, public fields are not very common in the std-library and this also applies to getters, I'd leave out those checks. |
|
I've commented on rust-lang/wg-allocators#2 (comment). If there are no concerns raised, I'll approve this PR in a couple of days (to give people time to respond). I think all of my concerns have been addressed now, and that this is a workable design. |
|
Many thanks to @Avi-D-coder and @varkor to make this possible. I think this is a big step! I have no concerns about the implementation. I don't think that it is easily possible to forbid the exposure of a type parameter, no matter if by a constructor or a getter/setter. This is basically like exposing API internals. I think that in most (all?) cases when using unstable type parameters, the API that uses this type parameter is also unstable (for example |
|
☔ The latest upstream changes (presumably #74710) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Avi-D-coder: just noticed that the tests don't cover unstable type defaults on enums or type aliases. Could you add a couple of those, and rebase, and then I'll approve! |
18 tasks
|
@Avi-D-coder any updates on this? |
|
@Dylan-DPC been exceptionally busy, I'll try to take a look this weekend. |
|
No worries, can understand :) Thanks for the update |
|
Ping from triage, there's merge conflicts now. |
crlf0710
added
S-waiting-on-author
|
Yeah I started resolving them, but couldn't finish it with limited time. |
|
@Avi-D-coder Ping from triage, any updates on this? |
crlf0710
added
S-waiting-on-author
|
Hi @Avi-D-coder, I've rebased your changes on the latest master here https://github.com/exrook/rust/tree/stability-generic-parameters-2 @varkor I've also tried adding some tests for type aliases and enums, see this commit: exrook@98eab09 Let me know if there's any cases I've missed, I basically just duplicated the struct tests for enums and aliases. |
|
@exrook: thanks, that looks great! If you open up a new pull request with those changes, I can approve that one (as @Avi-D-coder seems busy at the moment): the authors are tracked in the commits, so it doesn't matter so much which PR is merged. |
|
Closing in favour of #77118. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 27, 2020
…, r=varkor Stability annotations on generic parameters (take 2.5) Rebase of rust-lang#72314 + more tests Implements rust-lang/wg-allocators#2.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup of #65083 as an implementation of rust-lang/wg-allocators#2.
It's the same logic, but in one commit (to avoid rebasing).
It does not contain all of @varkor's clarifying comments and typo fixes.
This is still WIP, I'll summarize what works and followup on #65083 (comment) soon.