-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Clarified error note for usize range matching #146745
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
Conversation
Some changes occurred in match checking cc @Nadrieril |
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Need to also fix the tests which may just require an |
} else if ty.inner() == cx.tcx.types.isize { | ||
err.note(format!( | ||
"`{ty}` does not have fixed minimum and maximum values, so half-open \ | ||
"The minimum and maximum values of `{ty}` are not directly accessible, so half-open \ |
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 minimum and maximum values of `{ty}` are not directly accessible, so half-open \ | |
"the minimum and maximum values of `{ty}` are not directly accessible, so half-open \ |
https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-output-style-guide
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not really sure how to fix this failing check. It's being compared to the old one, even with the updated stderr. Seems like it's checking for stdout? |
You also need to update the error annotations within the test files themselves such as "NOTE |
Gotcha, thank you. |
if ty.inner() == cx.tcx.types.usize { | ||
err.note(format!( | ||
"`{ty}` does not have a fixed maximum value, so half-open ranges are \ | ||
"`{ty}::MAX` is not accessible as a fixed value, so half-open ranges are \ |
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.
Hmm. I'm not sure what "accessible" is intended to mean here, as in a sense of "the path to the item", it sure is accessible.
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.
Hmm. I'm not sure what "accessible" is intended to mean here, as in a sense of "the path to the item", it sure is accessible.
I think "accessible" means I need to access more sleep, because it looks like I was not comprehending anything I read.
Thinking of changing it to:
"usize::MAX
is not treated as exhaustive, so half-open ranges are necessary to match exhaustively"
and
"isize::MIN
and isize::MAX
are not treated as exhaustive, so half-open ranges are necessary to match exhaustively"
Or "considered" instead of "treated as"
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.
Mm. Maybe it would be good to think about this example:
Notice how usize::MAX
is... well, no longer "actually" usize::MAX
.
usize::MAX
is a library constant and remains so which also means shadowing it is fair game. It's a bit of a case of "I love it when I can implement the same things as the standard library does! ...No, wait, not like that 😭"
So usize
patterns do not consider any particular constant as forming the upper bound of the type. This also handles the case, as mentioned in the issue, of
const USIZE_MAX: usize = usize::MAX;
Which arguably should be treated as the exhaustive upper bound if we treat usize::MAX
also, but at what cost of compiler complexity and code correctness? Should it differ if one copies the literal there?
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.
You don't need to present an elaborate justification about compiler implementation, mind, indeed I expect most are not interested in that, I'm just noting some things.
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 considered elaborating on why this doesn't work the way it intuitively should, but decided that it wouldn't add all that much value in the end. The rationale being that knowing about the exact compiler quirk won't make it compile any better than the suggested fix (which might have some room for improvement here?) or, y'know, making work as it theoretically should on the compiler level.
Thanks! I feel like this could have a future improvement but I think this is also better than what we currently have so I think I will r+ it. Maybe someone will open a new issue and we'll revisit this someday. @bors r+ rollup |
@bors r- |
my brain. @helldawg Remembered one last thing: squash commits? |
This comment has been minimized.
This comment has been minimized.
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. |
Welp, that took a good 7 open tabs and 20 minutes. Good news is that the commits are squashed, and 700 other commits aren't tacked on for good measure. |
sorry. ^^; @bors r+ rollup |
Clarified error note for usize range matching Fixes rust-lang#146476 This is kinda rough, but it gets the point across a little better and stays short.
Rollup of 16 pull requests Successful merges: - #142139 (Include additional hashes in src/stage0) - #146745 (Clarified error note for usize range matching) - #146763 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 5)) - #146788 (chore: removes deprecated discord.) - #146942 ([rustdoc] Finish getting rid of usages `write_str`) - #147002 (rustdoc-search: stringdex update with more packing) - #147061 (fix rebasing cycle heads when not reaching a fixpoint) - #147066 (Fix tracking issue number for feature(macro_attr)) - #147081 (doc: fix a typo in platform-support.md) - #147082 (formatting_options: fix alternate docs 0b/0o mixup) - #147086 (compiletest: Use `PanicHookInfo::payload_as_str` now that it's stable in beta) - #147092 (Do not compute optimized MIR if code does not type-check.) - #147093 (redox: switch to colon as path separator) - #147095 (Library: Remove remaining private `#[repr]` workarounds) - #147098 (Add auto extra-checks in pre-push hook) - #147110 (Fix typo) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 14 pull requests Successful merges: - #142139 (Include additional hashes in src/stage0) - #146745 (Clarified error note for usize range matching) - #146763 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 5)) - #146788 (chore: removes deprecated discord.) - #146942 ([rustdoc] Finish getting rid of usages `write_str`) - #147061 (fix rebasing cycle heads when not reaching a fixpoint) - #147066 (Fix tracking issue number for feature(macro_attr)) - #147081 (doc: fix a typo in platform-support.md) - #147082 (formatting_options: fix alternate docs 0b/0o mixup) - #147086 (compiletest: Use `PanicHookInfo::payload_as_str` now that it's stable in beta) - #147093 (redox: switch to colon as path separator) - #147095 (Library: Remove remaining private `#[repr]` workarounds) - #147098 (Add auto extra-checks in pre-push hook) - #147110 (Fix typo) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #146476
This is kinda rough, but it gets the point across a little better and stays short.
r? @workingjubilee
@rustbot label A-diagnostics A-exhaustiveness-checking A-patterns