Skip to content
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

Make the BUG_REPORT_URL configurable for tools #109486

Closed
jyn514 opened this issue Mar 22, 2023 · 8 comments · Fixed by #110989
Closed

Make the BUG_REPORT_URL configurable for tools #109486

jyn514 opened this issue Mar 22, 2023 · 8 comments · Fixed by #110989
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 22, 2023

Right now, install_ice_hook hard-codes the bug report url to "https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md":

report_ice(info, BUG_REPORT_URL);

For tools, this is not correct; rustdoc at least wants T-rustdoc instead of T-compiler, and other tools want different repositories altogether. I'm opening this bug here because it's relevant to all tools, even though the original context was for clippy.

Fixing this seems a little tricky since DEFAULT_HOOK can't take an argument and doesn't have access to TyCtxt, but maybe we can solve this with thread-locals or something similar?

cc @rust-lang/rustdoc @rust-lang/clippy @rust-lang/miri @rust-lang/rustfmt
_Originally posted by @ingomancer in rust-lang/rust-clippy#10529

@jyn514 jyn514 added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Mar 22, 2023
@flip1995
Copy link
Member

Clippy already sets its own URL, so that is already possible. However Clippy needs to do a lot to do that:

const BUG_REPORT_URL: &str = "https://github.com/rust-lang/rust-clippy/issues/new";
type PanicCallback = dyn Fn(&panic::PanicInfo<'_>) + Sync + Send + 'static;
static ICE_HOOK: LazyLock<Box<PanicCallback>> = LazyLock::new(|| {
let hook = panic::take_hook();
panic::set_hook(Box::new(|info| report_clippy_ice(info, BUG_REPORT_URL)));
hook
});
fn report_clippy_ice(info: &panic::PanicInfo<'_>, bug_report_url: &str) {
// Invoke our ICE handler, which prints the actual panic message and optionally a backtrace
(*ICE_HOOK)(info);
// Separate the output with an empty line
eprintln!();
let fallback_bundle = rustc_errors::fallback_fluent_bundle(rustc_driver::DEFAULT_LOCALE_RESOURCES.to_vec(), false);
let emitter = Box::new(rustc_errors::emitter::EmitterWriter::stderr(
rustc_errors::ColorConfig::Auto,
None,
None,
fallback_bundle,
false,
false,
None,
false,
false,
rustc_errors::TerminalUrl::No,
));
let handler = rustc_errors::Handler::with_emitter(true, None, emitter);
// a .span_bug or .bug call has already printed what
// it wants to print.
if !info.payload().is::<rustc_errors::ExplicitBug>() {
let mut d = rustc_errors::Diagnostic::new(rustc_errors::Level::Bug, "unexpected panic");
handler.emit_diagnostic(&mut d);
}
let version_info = rustc_tools_util::get_version_info!();
let xs: Vec<Cow<'static, str>> = vec![
"the compiler unexpectedly panicked. this is a bug.".into(),
format!("we would appreciate a bug report: {bug_report_url}").into(),
format!("Clippy version: {version_info}").into(),
];
for note in &xs {
handler.note_without_error(note.as_ref());
}
// If backtraces are enabled, also print the query stack
let backtrace = env::var_os("RUST_BACKTRACE").map_or(false, |x| &x != "0");
let num_frames = if backtrace { None } else { Some(2) };
interface::try_print_query_stack(&handler, num_frames);
}

Definitely worth improving the situation here.

@jyn514
Copy link
Member Author

jyn514 commented Mar 22, 2023

Clippy already sets its own URL, so that is already possible. However Clippy needs to do a lot to do that:

Ah hmm, looks like the original problem was a bug in rustfix:

after fixes were automatically applied the compiler reported errors within these files:

  * src/main.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!

I'll open a separate issue there.

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

@rust-lang/miri the way I'm implementing this requires a change to the miri subtree in rust-lang/rust. What URL would you prefer for the bug report, just https://github.com/rust-lang/miri/issues/new ?

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

#110989 - looks like rustfmt isn't using install_ice_hook today? Happy to add it, but I'll make the PR to the rust-lang/rustfmt repo instead of doing it here.

@saethlin
Copy link
Member

In my experience, ICEs in Miri are just as often rustc bugs. I don't care if users report them in either place.

@calebcartwright
Copy link
Member

#110989 - looks like rustfmt isn't using install_ice_hook today? Happy to add it, but I'll make the PR to the rust-lang/rustfmt repo instead of doing it here.

Do you have a ballpark feel for the size of the change? FWIW I'd be okay with making the rustfmt update in tree if it's reasonably small and logically part of a batch of related in-tree changes

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

about 3 lines of code :) most of which will be the URL itself

@calebcartwright
Copy link
Member

about 3 lines of code :) most of which will be the URL itself

sgtm, feel free to go ahead 👍

Manishearth added a commit to Manishearth/rust that referenced this issue May 4, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc `@rust-lang/clippy`

Fixes rust-lang#109486.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 4, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ``@rust-lang/clippy``

Fixes rust-lang#109486.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 6, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ```@rust-lang/clippy```

Fixes rust-lang#109486.
@bors bors closed this as completed in 8ec84dd May 6, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue May 7, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ````@rust-lang/clippy````

Fixes rust-lang/rust#109486.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 20, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ````@rust-lang/clippy````

Fixes rust-lang/rust#109486.
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this issue Jun 20, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ````@rust-lang/clippy````

Fixes rust-lang/rust#109486.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants