Stop telling people to submit bugs for internal feature ICEs #116818

Noratrieb · 2023-10-16T20:16:00Z

This keeps track of usage of internal features, and changes the message to instead tell them that using internal features is not supported.

I thought about several ways to do this but now used the explicit threading of an Arc<AtomicBool> through Session. This is not exactly incremental-safe, but this is fine, as this is set during macro expansion, which is pre-incremental, and also only affects the output of ICEs, at which point incremental correctness doesn't matter much anyways.

See MCP 620.

rustbot · 2023-10-16T20:16:10Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

matthiaskrgr · 2023-10-16T20:21:45Z

compiler/rustc_driver_impl/messages.ftl

@@ -1,5 +1,6 @@
 driver_impl_ice = the compiler unexpectedly panicked. this is a bug.
 driver_impl_ice_bug_report = we would appreciate a bug report: {$bug_report_url}
+driver_impl_ice_bug_report_internal_feature = using internal features is not supported and is expected to lead to internal compiler errors


micro nit: maybe something like "usage of internal features is unsupported and know to cause internal compiler errors" ?

I like that wording better, or we could also soften it a bit more "and known to cause internal compiler errors when used incorrectly". I'm happy if people use internal features as long as they know they're mostly on their own if they opt to do so.

I have a slight preference towards "expected to" instead of "known to", because it's not just that we know that it's broken and don't care, we want it to be broken and deliberately add bug!()s.

compiler/rustc_interface/src/interface.rs

src/tools/miri/src/bin/miri.rs

rustbot · 2023-10-17T16:24:29Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

Manishearth · 2023-10-17T16:52:42Z

We should also detect when the crate being built is std (etc) and show the ICE warning then.

I've had multiple experiences in the past where build-std spews a bunch of weird errors I don't understand, for platforms like cortex-M that people tend to not care much about.

Noratrieb · 2023-10-17T17:06:41Z

I see. If someone's -Zbuild-std fails to build, then that does sound like something that should be reported. Seeing "this is not supported and expected to crash" isn't really right in that case.
But on the other hand, a contributor to the standard library might be playing with intrinsics and break something, in which case we do want to tell them to not open a bug.

I could special case the core/alloc/std (the most relevant standard library crates) crate-names to not set the flag for them to make sure that something like build-std still sends people an error - although in practice something like that is probably a libs issue and not a compiler bug, so the compiler isn't exactly wrong :D.

WaffleLapkin · 2023-10-17T17:53:27Z

I think if you are working on std/compiler you should know what to report and what to not report, so special-casing core/alloc/std should be fine.

Manishearth · 2023-10-17T18:05:39Z

But on the other hand, a contributor to the standard library might be playing with intrinsics and break something, in which case we do want to tell them to not open a bug.

I mean, (a) they would probably know this is the case and (b) "someone filed a bug they shouldn't have" is the issue I would prefer every time over "someone didn't file a bug". We don't need to solve this tradeoff perfectly.

wesleywiser

r=me with or without nit 🙂

wesleywiser · 2023-10-24T14:16:38Z

compiler/rustc_expand/src/config.rs

+                // The bug is probably in the standard library and not the compiler in that case, but that doesn't
+                // really matter - we want a bug report.
+                if features.internal(name)
+                    && ![sym::core, sym::alloc, sym::std].contains(&crate_name)


Nit: there's a few other crates that probably make sense to include like panic_abort, panic_unwind and compiler_builtins

compiler_builtins sounds reasonable, I'm gonna leave out the panic runtimes since they're so small

This keeps track of usage of internal features, and changes the message to instead tell them that using internal features is not supported. See MCP 620.

Noratrieb · 2023-10-25T21:42:25Z

@bors r=wesleywiser

bors · 2023-10-25T21:42:27Z

📌 Commit 9d42b1e has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-10-26T02:08:11Z

⌛ Testing commit 9d42b1e with merge 6d674af...

bors · 2023-10-26T04:03:44Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 6d674af to master...

rust-timer · 2023-10-26T05:45:25Z

Finished benchmarking commit (6d674af): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 304.41 MiB -> 304.46 MiB (0.01%)

…s, r=wesleywiser Stop telling people to submit bugs for internal feature ICEs This keeps track of usage of internal features, and changes the message to instead tell them that using internal features is not supported. I thought about several ways to do this but now used the explicit threading of an `Arc<AtomicBool>` through `Session`. This is not exactly incremental-safe, but this is fine, as this is set during macro expansion, which is pre-incremental, and also only affects the output of ICEs, at which point incremental correctness doesn't matter much anyways. See [MCP 620.](rust-lang/compiler-team#596) ![image](https://github.com/rust-lang/rust/assets/48135649/be661f05-b78a-40a9-b01d-81ad2dbdb690)

rustbot assigned wesleywiser Oct 16, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 16, 2023

matthiaskrgr reviewed Oct 16, 2023

View reviewed changes

WaffleLapkin reviewed Oct 16, 2023

View reviewed changes

compiler/rustc_interface/src/interface.rs Outdated Show resolved Hide resolved

src/tools/miri/src/bin/miri.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the stop-submitting-bug-reports branch from 9b125b5 to 1b2848b Compare October 17, 2023 16:24

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the stop-submitting-bug-reports branch from 1b2848b to daecd67 Compare October 17, 2023 16:52

Noratrieb force-pushed the stop-submitting-bug-reports branch from daecd67 to eb42037 Compare October 17, 2023 16:53

Noratrieb force-pushed the stop-submitting-bug-reports branch from eb42037 to 788ae5a Compare October 21, 2023 18:34

wesleywiser approved these changes Oct 24, 2023

View reviewed changes

Noratrieb force-pushed the stop-submitting-bug-reports branch from 788ae5a to 9d42b1e Compare October 25, 2023 21:22

Stop telling people to submit bugs for internal feature ICEs

9d42b1e

This keeps track of usage of internal features, and changes the message to instead tell them that using internal features is not supported. See MCP 620.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2023

bors merged commit 6d674af into rust-lang:master Oct 26, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 26, 2023

Noratrieb deleted the stop-submitting-bug-reports branch October 26, 2023 06:20

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop telling people to submit bugs for internal feature ICEs #116818

Stop telling people to submit bugs for internal feature ICEs #116818

Noratrieb commented Oct 16, 2023

rustbot commented Oct 16, 2023

matthiaskrgr Oct 16, 2023

compiler-errors Oct 16, 2023

Noratrieb Oct 17, 2023

This comment has been minimized.

rustbot commented Oct 17, 2023

This comment has been minimized.

Manishearth commented Oct 17, 2023

Noratrieb commented Oct 17, 2023

WaffleLapkin commented Oct 17, 2023

Manishearth commented Oct 17, 2023

wesleywiser left a comment

wesleywiser Oct 24, 2023

Noratrieb Oct 25, 2023

Noratrieb commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

Stop telling people to submit bugs for internal feature ICEs #116818

Stop telling people to submit bugs for internal feature ICEs #116818

Conversation

Noratrieb commented Oct 16, 2023

rustbot commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Oct 17, 2023

This comment has been minimized.

Manishearth commented Oct 17, 2023

Noratrieb commented Oct 17, 2023

WaffleLapkin commented Oct 17, 2023

Manishearth commented Oct 17, 2023

wesleywiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

Overall result: no relevant changes - no action needed