-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Add a detailed note for missing comma typo w/ FRU syntax #104504
Conversation
r? @lcnr (rustbot has picked a reviewer for you, use r? to override) |
cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki |
@@ -0,0 +1,8 @@ | |||
hir_typeck_fru_note = this expression may have been misinterpreted as a `..` range expression | |||
hir_typeck_fru_expr = this expression does not end in a comma... | |||
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update 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.
"functional record update syntax" is pretty heavy on jargon (and this is an error beginners will regularly see). Google search for it turns up nothing unless you add Rust, in which case you get this RFC: https://rust-lang.github.io/rfcs/2528-type-changing-struct-update-syntax.html
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.
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update syntax | |
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update syntax, used to set the remaining fields of a struct |
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.
This part of the error message is already pretty long, and it doesn't wrap well -- if you're against the jargon, I would rather remove everything after instead of
.
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'm also not too keen on the duplication of "used to set the remaining fields of a struct" between the note and the suggestion's "to set the remaining fields"
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 see your point! I think it's better left as is then.
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.
Maybe "struct update syntax"? I've heard that quite often when talking about FRU.
Applicability::MaybeIncorrect, | ||
); | ||
.ok() | ||
.filter(|s| s.len() < 25 && !s.contains(|c: char| c.is_control())); |
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.
As someone new to this part of the codebase, this line is unclear to me. What's s
, and why do we filter it like this? Why 25?
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.
This is to avoid rendering very long expressions in "to set the remaining fields in $EXPR
"
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.
25 is an arbitrary number.
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.
And is_control
is to avoid rendering something like:
to set the remaining fields in `a
.b`
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.
Makes sense :) Perhaps leave a comment to this effect?
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.
Sure thing.
📌 Commit b85b47cfd6c17acf21c8e3d6de3f543f3dbef942 has been approved by It is now in the queue for this repository. |
@bors r- i still got some changes to make |
b85b47c
to
bb0cb9a
Compare
@bors r=estebank |
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#101310 (Clarify and restrict when `{Arc,Rc}::get_unchecked_mut` is allowed.) - rust-lang#104461 (Fix building of `aarch64-pc-windows-gnullvm`) - rust-lang#104487 (update ntapi dep to remove future-incompat warning) - rust-lang#104504 (Add a detailed note for missing comma typo w/ FRU syntax) - rust-lang#104581 (rustdoc: remove unused JS IIFE from main.js) - rust-lang#104632 (avoid non-strict-provenance casts in libcore tests) - rust-lang#104634 (move core::arch into separate file) - rust-lang#104641 (replace unusual grammar) - rust-lang#104643 (add examples to chunks remainder methods. ) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Thanks to @pierwill for working on this with me!
Fixes #104373, perhaps @alice-i-cecile can comment on the new error for the example provided on that issue -- feedback is welcome.