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

Don't even parse an intrinsic unless the feature gate is enabled #123603

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Apr 7, 2024

Don't return true in tcx.is_intrinsic if the function is defined locally and #![feature(intrinsics)] is not enabled. This is a slightly more general fix than #123526, since #123587 shows that we have simplifying assumptions about intrinsics elsewhere in the compiler.

This will make the code ICE again if the user enables #[feature(intrinsics)], but I kind of feel like if we want to fix that, we should make the INTERNAL_FEATURES lint Deny again. Perhaps we could do that on non-nightly compilers. Or we should just stop compilation altogether if they have #![feature] enabled on a non-nightly compiler.

As for the UX of real cases of hitting these ICEs, I believe pretty strongly that if a compiler/stdlib dev is modifying internal intrinsics (intentionally, like when making a change to rustc) we have no guarantee to make the ICE better looking for them. Honestly, not spitting out a stack trace is probably a disservice to the people who hit those ICEs in that case.

r? @Nilstrieb @estebank

@rustbot 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. labels Apr 7, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me gated on only one thing: do we already have any tests where intrinsics are enabled and the function doesn't exist? I'm concerned that unrecognized intrinsic function: f1 is not being tested in any other test.

@compiler-errors
Copy link
Member Author

I'll add it if not

@compiler-errors
Copy link
Member Author

We already have tests for this, e.g. E0093.rs

@bors r=estebank rollup

@bors
Copy link
Contributor

bors commented Apr 15, 2024

📌 Commit 651d02a has been approved by estebank

It is now in the queue for this repository.

@bors 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 Apr 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#123016 (Remove `TypeVariableOriginKind` and `ConstVariableOriginKind`)
 - rust-lang#123462 (Cleanup: Rename `ModSep` to `PathSep`)
 - rust-lang#123603 (Don't even parse an intrinsic unless the feature gate is enabled)
 - rust-lang#123926 (Fix pretty HIR for anon consts in diagnostics)
 - rust-lang#123973 (crashes: readme: add reminder to add Fixes #abcde to prs to automatically close issues.)
 - rust-lang#123984 (sanitizers: Add rustc_sanitizers to triagebot.toml)
 - rust-lang#123989 (Just use `type_dependent_def_id` to figure out what the method is for an expr)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1ad9fea into rust-lang:master Apr 16, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 16, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2024
Rollup merge of rust-lang#123603 - compiler-errors:no-intrinsic, r=estebank

Don't even parse an intrinsic unless the feature gate is enabled

Don't return true in `tcx.is_intrinsic` if the function is defined locally and `#![feature(intrinsics)]` is not enabled. This is a slightly more general fix than rust-lang#123526, since rust-lang#123587 shows that we have simplifying assumptions about intrinsics elsewhere in the compiler.

This will make the code ICE again if the user **enables** `#[feature(intrinsics)]`, but I kind of feel like if we want to fix that, we should make the `INTERNAL_FEATURES` lint `Deny` again. Perhaps we could do that on non-nightly compilers. Or we should just stop compilation altogether if they have `#![feature]` enabled on a non-nightly compiler.

As for the UX of *real* cases of hitting these ICEs, I believe pretty strongly that if a compiler/stdlib dev is modifying internal intrinsics (intentionally, like when making a change to rustc) we have no guarantee to make the ICE better looking for them. Honestly, *not* spitting out a stack trace is probably a disservice to the people who hit those ICEs in that case.

r? `@Nilstrieb` `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants