Add a new type parameter to new Range types#155457
Open
nik-rev wants to merge 1 commit intorust-lang:mainfrom
Open
Add a new type parameter to new Range types#155457nik-rev wants to merge 1 commit intorust-lang:mainfrom
Range types#155457nik-rev wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
Contributor
|
I want to mention that without resolving the issue that occurs in the attempt I made, it's possible that certain things might not compile correctly within the rust repo when we upgrade or switch to the new range types. For example this was a failure from my draft PR: 
We need to give some sort of treatment to Range to infer literal values like 0 or non-concrete types like |
Contributor
Author
In your example, this was the problematic line: Box::new(0..lane_count)Issue is that I think this is fine, however, because unlike the previous attempt - this PR does not break any existing code. You have to manually upgrade to use the new type in editions older than Edition 2027, at which point it can just be fixed manually For Edition 2027, yes, code like Even if that is still a problem for us, we should still merge this PR as soon as possible, to minimize how many people could possibly be broken by this who are on |
Contributor
But that's the thing, I feel like if we want to have this PR merged, this need to handle those inference failures in the compiler as well. Someone from the libs team or compiler team may want to chime in on whether or not it's acceptable for us to have this behavior in the new |
Contributor
Author
Edition 2027 is still a while away, and we can revert these changes before we stabilize that edition. Merging this PR as fast as possible means:
We are giving ourselves a safety net by merging this now, even if we don't 100% know how we will deal with the inference failures, we have a lot of time to figure out. |
Contributor
|
Okay... I really think this is where libs or compiler team has to chime in for their thoughts. Also, as far as I'm aware from the Rust Project team @pitaj is a clippy contributor and on the triage team, I don't think he should be the one approving this PR since this is a libs change (correct me if I'm wrong). Someone from the libs team on review rotation should be reviewing/approving this PR. |
Collaborator
|
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
rustbot
added
the
T-rust-analyzer
Contributor
|
Nominating for @rust-lang/libs-api: half a year ago you said you'd be fine to add this if inference doesn't blow up rust-lang/libs-team#653 (comment). Based on the comments here it seems like there are cases that this can make less ergonomic once we have the stable range syntax. It's probably not-impossible (though unlikely) to break use of Anyway, seems like a now-or-never decision whether we want to carve this out now or require specialized compiler support to land first. Personally I'm +0.9, not +1 because I don't know what rustfix will be able to do at the edition change in cases where inference doesn't work (guess it could be fine to require manual intervention?). Some discussion at the ACP rust-lang/libs-team#653 @rustbot label +I-libs-api-nominated |
rustbot
added
the
I-libs-api-nominated
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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 adds a new type parameter to the
Rangetypes:Previous attempt:
https://github.com/rust-lang/rust/pull/151334/changes#diff-4c5209c8bab47c4b4cc495b2001cbf4f7ed8514640e875b01067fd5ed12f3b66
This PR tried to add the new type parameter to the old
core::ops::Rangetypes. However, it didn't work out due to type inference failures.We can add it to the new
Rangetypes, because they are just being stabilized, so there's not much usage of them, and it's unlikely that people will be broken by it.ACP:
RangeBounds<Start, End = Start>and all existing range types libs-team#653 (comment)Tracking Issue:
new_range_end_bound#155456Question: Should this be a beta backport? See comment by @pitaj:
This change may cause type inference failures for those upgrading from
1.95to1.97, assuming this PR hits stable by then. If we instead do a beta backport, this will halve the time window for possible inference regressions.r? pitaj