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

Policy for ICEs on incorrect usage of internal-only features #620

Closed
1 of 3 tasks
Noratrieb opened this issue Apr 20, 2023 · 3 comments
Closed
1 of 3 tasks

Policy for ICEs on incorrect usage of internal-only features #620

Noratrieb opened this issue Apr 20, 2023 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@Noratrieb
Copy link
Member

Noratrieb commented Apr 20, 2023

Proposal

When using internal features like lang_items or intrinsics, rustc is very panicky. For examples, see rust-lang/rust#110538, rust-lang/rust#102465, rust-lang/rust#101964.

rustc makes many assumptions about the shape and presence of lang items and intrinsics. These assumptions are deep within the compiler and make the code simpler or faster. Trying to give proper errors for these broken assumptions would increase the code complexity of the compiler.

These errors are only being hit either by people working on the lang items (which will be people with some knowledge of the compiler, so they should be fine with the ICEs) or by people just playing around, which we don't need to support when it costs us something. Additionally, after #596 users using these panicky features will be made aware of what they're doing.

Therefore, I propose the following policy when dealing with ICEs/confusing errors from problems with the declared lang items or intrinsics in the source code:

ICEs or confusing diagnostics when using internal-only features like intrinsics or lang items incorrectly are not considered rustc bugs. Therefore, diagnostics improvements or ICE fixes for issues related to usage of internal only features are generally not accepted unless they are particularly useful in general. For example, the compiler currently validates that the item kind and amount of generic args of lang items matches, the code for which is fairly simple and useful for all lang items so we will keep it. In particular, one-off fixes for particular assumptions about lang items or intrinsics that introduce additional complexity into the compiler are not accepted. Error or ICE messages on incorrect usages of internal-only features are intended for compiler developers and not general users.

Exceptions could always be made on a case-by-case basis as deemed appropriate by T-compiler or compiler contributors, but this policy is there to have something to point at when closing PRs or issues as wontfix.

As a concrete example of an application of this policy, this PR of mine would have been denied: rust-lang/rust#104216. It added some complexity and a test for a broken lang item definition.

We should PR the policy do the dev guide if it is accepted. We may also want to skim the test suite for tests that test the absence of ICEs on broken lang item definitions like added in the PR shown above and delete those tests.

Mentors or Reviewers

@compiler-errors asked me to make this and helped with the wording

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@Noratrieb Noratrieb added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Apr 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 20, 2023
@Noratrieb Noratrieb changed the title no_core/lang_items/intrinsics ICE policy Policy for ICEs on incorrect usage of internal-only features Apr 20, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 20, 2023

@rustbot second

@apiraino
Copy link
Contributor

apiraino commented May 2, 2023

@rustbot label -final-comment-period +major-change-accepted

@apiraino apiraino closed this as completed May 2, 2023
@rustbot rustbot added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels May 2, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 6, 2023
… r=cjgillot

More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items

In rust-lang#111264, a user added a new item to the `Future` trait, but the code in [`resolve_associated_item`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ty_utils/instance/fn.resolve_associated_item.html) implicitly assumes that the `Future` trait is defined with only one method (`Future::poll`) and treats the generator body as the implementation of that method.

This PR adds some debug assertions to make sure that that new methods defined on `Future`/`Generator`/etc. don't accidentally resolve to the wrong item when they are added, and adds a helpful comment guiding a compiler dev (or curious `#![no_core]` user) to what must be done to support adding new associated items to these built-in implementations.

I am open to discuss whether a test should be added, but I chose against it because I opted to make these `bug!()`s instead of, e.g., diagnostics or fatal errors. Arguably it doesn't need a test because it's not a bug that can be triggered by an end user, and internal-facing misuses of core kind of touch on rust-lang/compiler-team#620 -- however, I think the assertions I added in this PR are still a very useful way to make sure this bug doesn't waste debugging resources down the line.

Fixes rust-lang#111264
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 6, 2023
… r=cjgillot

More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items

In rust-lang#111264, a user added a new item to the `Future` trait, but the code in [`resolve_associated_item`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ty_utils/instance/fn.resolve_associated_item.html) implicitly assumes that the `Future` trait is defined with only one method (`Future::poll`) and treats the generator body as the implementation of that method.

This PR adds some debug assertions to make sure that that new methods defined on `Future`/`Generator`/etc. don't accidentally resolve to the wrong item when they are added, and adds a helpful comment guiding a compiler dev (or curious `#![no_core]` user) to what must be done to support adding new associated items to these built-in implementations.

I am open to discuss whether a test should be added, but I chose against it because I opted to make these `bug!()`s instead of, e.g., diagnostics or fatal errors. Arguably it doesn't need a test because it's not a bug that can be triggered by an end user, and internal-facing misuses of core kind of touch on rust-lang/compiler-team#620 -- however, I think the assertions I added in this PR are still a very useful way to make sure this bug doesn't waste debugging resources down the line.

Fixes rust-lang#111264
RalfJung pushed a commit to RalfJung/miri that referenced this issue May 7, 2023
More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items

In #111264, a user added a new item to the `Future` trait, but the code in [`resolve_associated_item`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ty_utils/instance/fn.resolve_associated_item.html) implicitly assumes that the `Future` trait is defined with only one method (`Future::poll`) and treats the generator body as the implementation of that method.

This PR adds some debug assertions to make sure that that new methods defined on `Future`/`Generator`/etc. don't accidentally resolve to the wrong item when they are added, and adds a helpful comment guiding a compiler dev (or curious `#![no_core]` user) to what must be done to support adding new associated items to these built-in implementations.

I am open to discuss whether a test should be added, but I chose against it because I opted to make these `bug!()`s instead of, e.g., diagnostics or fatal errors. Arguably it doesn't need a test because it's not a bug that can be triggered by an end user, and internal-facing misuses of core kind of touch on rust-lang/compiler-team#620 -- however, I think the assertions I added in this PR are still a very useful way to make sure this bug doesn't waste debugging resources down the line.

Fixes #111264
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 30, 2024
…=Nilstrieb

Remove some unnecessary check logic for lang items in HIR typeck

Obvious bugs with `#[no_core]` do not deserve customized recovery logic, since they are bugs we do not expect users to ever encounter, and if users are experimenting with `#[no_core]`, they should really be familiar with the compiler implementation.

These error recoveries are implemented now only where issues have been reported in the past, rather than systematically validating lang items.

See rust-lang/compiler-team#620
> In particular, one-off fixes for particular assumptions about lang items or intrinsics that introduce additional complexity into the compiler are not accepted.

r? Nilstrieb
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 30, 2024
Rollup merge of rust-lang#120476 - compiler-errors:lang-items-yeet, r=Nilstrieb

Remove some unnecessary check logic for lang items in HIR typeck

Obvious bugs with `#[no_core]` do not deserve customized recovery logic, since they are bugs we do not expect users to ever encounter, and if users are experimenting with `#[no_core]`, they should really be familiar with the compiler implementation.

These error recoveries are implemented now only where issues have been reported in the past, rather than systematically validating lang items.

See rust-lang/compiler-team#620
> In particular, one-off fixes for particular assumptions about lang items or intrinsics that introduce additional complexity into the compiler are not accepted.

r? Nilstrieb
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 18, 2024
…rochenkov

Add help to `hir_analysis_unrecognized_intrinsic_function`

To help remind forgetful people like me what step they forgot.

(If this just ICE'd, rust-lang/compiler-team#620 style, the stack trace would point me here, but since there's a "nice" error that information is lost.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 18, 2024
…rochenkov

Add help to `hir_analysis_unrecognized_intrinsic_function`

To help remind forgetful people like me what step they forgot.

(If this just ICE'd, rust-lang/compiler-team#620 style, the stack trace would point me here, but since there's a "nice" error that information is lost.)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 18, 2024
Rollup merge of rust-lang#121247 - scottmcm:intrinsic-reminder, r=petrochenkov

Add help to `hir_analysis_unrecognized_intrinsic_function`

To help remind forgetful people like me what step they forgot.

(If this just ICE'd, rust-lang/compiler-team#620 style, the stack trace would point me here, but since there's a "nice" error that information is lost.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 25, 2024
…, r=oli-obk

Remove crashes for misuses of intrinsics

All of these do not crash if the feature gate is removed. An ICE due *opting into* the intrinsics feature gate is not a bug that needs to be fixed, but instead a misuse of an internal-only API.

See rust-lang/compiler-team#620

The last two issues are already closed anyways, but:
Fixes rust-lang#97501
Fixes rust-lang#111699
Fixes rust-lang#101962
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2024
Rollup merge of rust-lang#128173 - compiler-errors:misused-intrinsics, r=oli-obk

Remove crashes for misuses of intrinsics

All of these do not crash if the feature gate is removed. An ICE due *opting into* the intrinsics feature gate is not a bug that needs to be fixed, but instead a misuse of an internal-only API.

See rust-lang/compiler-team#620

The last two issues are already closed anyways, but:
Fixes rust-lang#97501
Fixes rust-lang#111699
Fixes rust-lang#101962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants