-
Notifications
You must be signed in to change notification settings - Fork 13.8k
indexing: reword help #146585
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
indexing: reword help #146585
Conversation
This PR modifies |
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
2c57fd0
to
83d8e8f
Compare
// fixed expression: | ||
err.help( | ||
"tuples are indexed (like structs) with dot (`._`) not with brackets (`[_]`), \ | ||
element names are their positions: tuple = (tuple.0, tuple.1, ...)", |
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 feel like this line ("element names..") is a bit too extraneous. People trying to do x[0]
might not really be very confused on which indices each field ends up (and it is probably better to leave this role to some other place teaching tuples)
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've removed this bit, and also the reference to structs.
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.
@rustbot ready
97af26a
to
62b3b53
Compare
tests/ui/issues/issue-27842.stderr
Outdated
| ^^^ | ||
| | ||
= help: to access tuple elements, use tuple indexing syntax (e.g., `tuple.0`) | ||
= help: tuples are indexed with dot (`._`): tuple = (tuple.0, tuple.1, ...) |
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 still don't think this message is better than the original. I like your changes to the error code docs, but the ._
confuses me and tuple =
in the example too. I'd approve this if the error message and help stayed the same as it was.
Your changes to the case where you provide an integer as index is improved though, I like the "to access element 0, use .0
" :)
@rustbot author
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.
Thanks for reviewing!
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've made some small changes to address your comments. Let me know if it is any better!
@rustbot ready
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 basic idea is to use the fact that a tuple is equal to its elements as a harness for the tuple index syntax...
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.
well I get that that's what it's trying to convey, I just don't think it's any clearer than giving a single example like before
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.
@rustbot author
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.
again, I think your change to the message when providing an integer as index is good! I just really don't think this part is very clear while I think the way it was was perfectly clear
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.
Alright I took out that idea, and now it's much closer to what was there before.
@rustbot ready
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.
see above
@hkBst I think this is a cool change, still feel like fixing it up? |
@jdonszelmann I'll get back to it in a bit. Just giving it some time, so I can look at it fresh. :) |
Very good! No rush, just saw it in my review queue hehe |
This comment has been minimized.
This comment has been minimized.
6343920
to
c70dbd4
Compare
This comment has been minimized.
This comment has been minimized.
c70dbd4
to
dd1b7ad
Compare
dd1b7ad
to
e2b7e03
Compare
tests/ui/issues/issue-27842.stderr
Outdated
| | ||
= help: to access tuple elements, use tuple indexing syntax (e.g., `tuple.0`) | ||
= help: tuples are indexed with a dot and a constant index, e.g.: `tuple.0` |
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.
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.e. I prefer the comma
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.
also it's not a constant. i.e. it can't be replaced by tuple.const{3 + 3}
or whatever (though that'd be cool). Genuinely, it was pretty good as it was! like, it is tuple indexing syntax, that's fundamentally what it is, and in the compiler it acts much more like an identifier than a constant. For example, it's perfectly valid to initialize a tuple struct like this:
struct Foo(u32);
fn main() {
let x = Foo {0: 3}
}
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're right that "constant" is perhaps not quite right. Perhaps "literal" or "literal integer"? Indexing a tuple may use "tuple indexing syntax", but that name does not seem to have much (any?) explanatory power, which is why I am trying to describe it instead of naming it.
My usage of "e.g.:" seems indeed to be unusual, but the colon can stand on its own I think.
Genuinely, it was pretty good as it was!
It is adequate, but I think we can do better. Perhaps "tuple elements are accessed" is better than "tuples are indexed" though?
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've pushed another version of 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.
@rustbot ready
Co-authored-by: beef <[email protected]>
e2b7e03
to
86f2d42
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. |
@bors r+ rollup |
indexing: reword help After looking at rust-lang#40850, I thought I'd try to improve wording around error E0608 a bit. Hopefully I've succeeded.
Rollup of 10 pull requests Successful merges: - #146281 (Support `#[rustc_align_static]` inside `thread_local!`) - #146535 (mbe: Implement `unsafe` attribute rules) - #146585 (indexing: reword help) - #147004 (Tweak handling of "struct like start" where a struct isn't supported) - #147221 (Forbid `//@ compile-flags: -Cincremental=` in tests) - #147225 (Don't enable shared memory by default with Wasm atomics) - #147227 (implement `Box::take`) - #147231 (Extending `#[rustc_force_inline]` to be applicable to inherent methods) - #147233 (Initialize llvm submodule if not already the case to run citool) - #147236 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
indexing: reword help After looking at rust-lang#40850, I thought I'd try to improve wording around error E0608 a bit. Hopefully I've succeeded.
indexing: reword help After looking at rust-lang#40850, I thought I'd try to improve wording around error E0608 a bit. Hopefully I've succeeded.
Rollup of 9 pull requests Successful merges: - #146281 (Support `#[rustc_align_static]` inside `thread_local!`) - #146535 (mbe: Implement `unsafe` attribute rules) - #146585 (indexing: reword help) - #147004 (Tweak handling of "struct like start" where a struct isn't supported) - #147221 (Forbid `//@ compile-flags: -Cincremental=` in tests) - #147225 (Don't enable shared memory by default with Wasm atomics) - #147227 (implement `Box::take`) - #147233 (Initialize llvm submodule if not already the case to run citool) - #147236 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 9 pull requests Successful merges: - rust-lang/rust#146281 (Support `#[rustc_align_static]` inside `thread_local!`) - rust-lang/rust#146535 (mbe: Implement `unsafe` attribute rules) - rust-lang/rust#146585 (indexing: reword help) - rust-lang/rust#147004 (Tweak handling of "struct like start" where a struct isn't supported) - rust-lang/rust#147221 (Forbid `//@ compile-flags: -Cincremental=` in tests) - rust-lang/rust#147225 (Don't enable shared memory by default with Wasm atomics) - rust-lang/rust#147227 (implement `Box::take`) - rust-lang/rust#147233 (Initialize llvm submodule if not already the case to run citool) - rust-lang/rust#147236 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
After looking at #40850, I thought I'd try to improve wording around error E0608 a bit. Hopefully I've succeeded.