-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Improve error message when writer is forgotten in write and writeln macro #109149
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @WaffleLapkin (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
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 the PR. Can you give this PR a more "functional" title -- something like, "improve error message when user forgets to pass a writer to write!(..)
", or something that describes what this pr does, rather than just what issue this addresses?
Then in the body of the PR, you can put fixes #108713
, and that'll link the issue appopriately.
"no {} named `{}` found for {} `{}` in the current scope", | ||
item_kind, | ||
item_name, | ||
rcvr_ty.prefix_string(self.tcx), | ||
ty_str_reported, | ||
); | ||
err = struct_span_err!( | ||
tcx.sess, | ||
args.unwrap().0.span, | ||
E0599, | ||
"cannot write into '{}'", | ||
ty_str_reported, | ||
); | ||
err.span_note( | ||
arg_span, | ||
"must implement 'io::Write', 'fmt::Write', or have a write_fmt method", | ||
); | ||
if writer_note.is_some() { | ||
err.span_help(arg_span.shrink_to_lo(), writer_note.unwrap()); | ||
} | ||
} else { | ||
err = struct_span_err!( | ||
tcx.sess, | ||
span, | ||
E0599, | ||
"no {} named `{}` found for {} `{}` in the current scope", | ||
item_kind, | ||
item_name, | ||
rcvr_ty.prefix_string(self.tcx), | ||
ty_str_reported, | ||
); | ||
} |
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 this would be a good opportunity to migrate this into a diagnostic 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.
Sorry not sure I'm understanding correctly, are you suggesting moving the missing writer portion into a diagnostic struct or moving the entire report_no_match_method_error into a diagnostic 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.
Probably both, into different diagnostic 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.
Gotcha, thanks
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.
If the other changes I made look fine I can get started working on this
This comment has been minimized.
This comment has been minimized.
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in src/tools/cargo cc @ehuss These commits modify compiler targets. Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in engine.rs, potentially modifying the public API of Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor The Miri subtree was changed cc @rust-lang/miri Some changes occurred in diagnostic error codes This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras This PR changes src/bootstrap/defaults/config.codegen.toml. If appropriate, please also update These commits modify the If this was intentional then you can ignore this comment. |
I'm sorry I did something wrong when I was trying to ff master |
This comment has been minimized.
This comment has been minimized.
Hi, you accidentally added a merge commit pulling in tons of other commits. Just use |
648960b
to
46ed86e
Compare
This comment has been minimized.
This comment has been minimized.
@rustbot label -A-testsuite, -A-meta, -T-bootstrap, -T-release, -T-infra, -WG-trait-system-refactor |
This comment has been minimized.
This comment has been minimized.
346e861
to
7c4077b
Compare
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.
couple of nit picks
struct_span_err!(self.tcx.sess, args.0.span, E0599, "cannot write into `{}`", ty_str); | ||
err.span_note( | ||
args.0.span, | ||
"must implement 'io::Write', 'fmt::Write', or have a write_fmt method", |
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, diagnostics usually use backticks
"must implement 'io::Write', 'fmt::Write', or have a write_fmt method", | |
"must implement `io::Write`, `fmt::Write`, or have a `write_fmt` method", |
7c4077b
to
81fdc97
Compare
I am getting the following error when I run ./x.py test on my machine:
I see that it's coming from write_fmt, is this likely because of my changes or should I report the bug? |
@mj10021 I don't think this is caused by your changes. first, try using |
struct_span_err!(self.tcx.sess, args.0.span, E0599, "cannot write into `{}`", ty_str); | ||
err.span_note( | ||
args.0.span, | ||
"must implement `io::Write`, `fmt::Write`, or have a write_fmt method", |
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.
"must implement `io::Write`, `fmt::Write`, or have a write_fmt method", | |
"must implement `io::Write`, `fmt::Write`, or have a `write_fmt` method", |
write_fmt
should be ticked too.
Yes, thank you, |
5d63f3f
to
5d7ce2b
Compare
Can you rebase and squash this into one commit? This commit stack is getting pretty long for a pretty self-contained change. |
Yes, absolutely |
879cce3
to
ccb982a
Compare
This comment has been minimized.
This comment has been minimized.
added ui test blessed stderrs fixed typo reblessed
ccb982a
to
ff88787
Compare
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 happy with this for now, though perhaps @WaffleLapkin has more comments so i'll leave it to waffle to approve (or re-assign to me) -- any follow-ups can be done in further prs
struct_span_err!(self.tcx.sess, args.0.span, E0599, "cannot write into `{}`", ty_str); | ||
err.span_note( | ||
args.0.span, | ||
"must implement `io::Write`, `fmt::Write`, or have a `write_fmt` method", |
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.
Can this message include a subject? E.g.: "a writer must..."? I'm worried that otherwise it might not be clear what the message is talking about.
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 looks good to me too. Improvements (like message tweaking that @mejrs suggested) can indeed be done in follow-ups. Thanks!
@bors r=compiler-errors,WaffleLapkin |
@bors rollup |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#109149 (Improve error message when writer is forgotten in write and writeln macro) - rust-lang#109367 (Streamline fast rejection) - rust-lang#109548 (AnnotationColumn struct to fix hard tab column numbers in errors) - rust-lang#109694 (do not panic on failure to acquire jobserver token) - rust-lang#109705 (new solver: check for intercrate mode when accessing the cache) - rust-lang#109708 (Specialization involving RPITITs is broken so ignore the diagnostic differences) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Modified write! macro error message when writer is forgotten as in issue #108713
Fixes #108713
r? @WaffleLapkin