Suggest add bounding value for RangeTo #147753
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @petrochenkov. Use |
| } | ||
|
|
||
| fn suggest_bounds_for_range_to_method( | ||
| &self, |
There was a problem hiding this comment.
This should only suggest 0 if the element type is integral.
Right now, you suggest (0..'f').rev() for (..'f').rev(). This affects all types that implement the Step trait and aren't integral.
There was a problem hiding this comment.
Also, should it be 0 if the element is signed?
There was a problem hiding this comment.
yeah, it's not good to suggest (0..-10), so I changed the code to only suggest 0 for integral and without negative sign, otherwise suggest a place hold _, not sure this is the best way, but I didn't come out with a better one.
it's complex to handle all scenario, for instance the code maybe (...val), while val is a negative value or some other types..
anyway, this is a MaybeIncorrect suggestion.
|
r? @fmease |
|
|
chenyukang
force-pushed
the
yukang-147749
branch
from
a137c1c to
29c1027
Compare
| | | ||
| LL | for _x in (_..'a').rev() {} | ||
| | + |
There was a problem hiding this comment.
I still kinda sceptic about this underscore as suggetstion honestly
I wonder if it will be possible to get a type to make suggestion like here?
This one is kinda nice and very noticeable for users
There was a problem hiding this comment.
you mean like this？
for _x in (/* start */..'a').rev() {}
+++++seems better, but the */ followed by .. maybe looks confusing?
There was a problem hiding this comment.
I asked in discord some opinions, so... hopefully we will get little more points of view on this
Yeah, I mean this, hm, looks a bit confusing I agree, maybe space can help, what do you think?
for _x in ( /* start */ ..'a').rev() {}
+++++There was a problem hiding this comment.
I think it's OK without the space, since I just realized it looks perfect in a terminal:
There was a problem hiding this comment.
Highlight is doing great job here, but still, do you think it reasonable to suggest 0 for integers, for example above 0..4 is i32 afaik, so it should be i32::MIN
I'd suggest to do comment placeholder for all types, just for consistency and should make implementation easier
There was a problem hiding this comment.
I guess starting from 0 is the most common scenario if the ending bound is an integer.
I am neutral on both choices here. Any thoughts on this? @fmease
There was a problem hiding this comment.
I'd use 0 for integers cause I think that's probably what the user intended. At least that was the case for me. Non integer cases seem less likely to occur in the first place. I guess char::MIN makes sense for chars.
There was a problem hiding this comment.
Well, yes, I do agree that 0 is most common scenario so let's keep it as it
char::MIN is \0, so I'm not sure if it's what users really have meant, most likely it's just a or A (depends on case)
chenyukang
force-pushed
the
yukang-147749
branch
2 times, most recently
from
f1795fa to
1a2f058
Compare
…method-error, r=nnethercote Code refactoring on hir report_no_match_method_error While working on rust-lang#147753, I found `report_no_match_method_error` now is too long for maintain, 1200 lines of code now: https://github.com/rust-lang/rust/blob/57ef8d642d21965304bde849bab4f389b4353e27/compiler/rustc_hir_typeck/src/method/suggest.rs#L589-L1736 this PR try to refactor it. I tried my best to group most related code into same places, but the logic here is still very complex, there are some variables across different functions, maybe we need more work to make it better understand. Maybe we could add a tidy check to avoid long spaghetti code. r? `@nnethercote`
chenyukang
force-pushed
the
yukang-147749
branch
from
1a2f058 to
038dd12
Compare
|
This PR was rebased onto a different master 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. |
…ror, r=nnethercote Code refactoring on hir report_no_match_method_error While working on rust-lang/rust#147753, I found `report_no_match_method_error` now is too long for maintain, 1200 lines of code now: https://github.com/rust-lang/rust/blob/57ef8d642d21965304bde849bab4f389b4353e27/compiler/rustc_hir_typeck/src/method/suggest.rs#L589-L1736 this PR try to refactor it. I tried my best to group most related code into same places, but the logic here is still very complex, there are some variables across different functions, maybe we need more work to make it better understand. Maybe we could add a tidy check to avoid long spaghetti code. r? `@nnethercote`
7 participants
Fixes #147749