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.
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
Supertrait item shadowing v2 #3624
Supertrait item shadowing v2 #3624
Changes from 3 commits
b32c37e
de6a4cf
95594f7
57892c5
c4cc0a8
f54263a
0872f23
566bbc9
8df733c
dc3e2fd
9d34dc0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is picking a different name not a reasonable path? The unavoidable bikeshedding around a new name is annoying, but it seems to me like a small, one-time cost compared to the permanent additional language complexity of this feature.
I also wonder how many times we anticipate to run into this problem in the future. Are there more examples aside from
Itertools::intersperse
? If we only ran into this problem once within 9 years of Rust being stable, the benefit of this feature seems very limited.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking a new name also has disadvantages other than than the time taken to pick it, users then have to learn the new method name and that it is semantically identical to the
Itertools::intersperse()
yet has a different name. This is not a one-time cost as it will effect all future users when, for example, searching docs for this method. This would be the case for all methods stabilized fromitertools
tostd
which might be quite a few if we had a reliable way to do such a migration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the problem. People who are using
intersperse
fromitertools
shouldn't expect the standard library to use the exact same name. And I assume few people rely on the unstable version instd
for production code, it would be unwise to opt into breaking changes for what amounts to a small ergonomics improvement (which a library can also provide).For regular Rust users, the function doesn't exist right now. In the future it may - its name is an open question.
I agree that this is something to consider, we don't want to have to pick the "second best" name for many functions in
std
. But first we should think concretely about which methods fromitertools
might actually make the jump intostd
. itertools has been around for a long time and I'm not aware of a big push to upstream many of its methods. Which indicates to me, there isn't that much need. But I'm happy to be convinced otherwise. Examples from other libraries besides itertools count as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that because itertools is used widely, std cannot upstream methods from itertools without causing lots of breakage in all crates currently using itertools. This is a perverse incentive which forces the "second best" issue you mentioned, and it's not limited to itertools, it happens whenever std lacks a function, someone implements an extension trait to add it in a crate (as they should), and everyone picks it up because it is really useful (as they should). The exact sequence of events which leads to strong evidence that something should be in std is also the sequence of events that blocks it from being added to std.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. The reason I'm pushing back is that to me, the feature doesn't seem to align with Rust's design principles. This new implicit default doesn't seem obviously correct to me. If I call a method that has two implementations, I would generally prefer the compiler yell at me rather than pick one without telling me. That's why I think we should be certain that we'll make good use of this feature before adding it.
But I'll admit this is a theoretical objection. In practice, the problem may never show up and then it's fine to add the feature. Pragmatism comes first. I guess I just agree with this comment. We should think about edge cases where this could go wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rfc mentions this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a lint would make sense if the migration-situation with intersperse is the only use case we expect this feature to be used in practice. But maybe some users will use the shadowing to implement a form of specialization? Or any other unrelated use case I can't think of right now. If that happens, it might lead to a discussion about whether the lint should be enabled by default or not. And if it ends up allow-by-default, it loses much of its value.