Skip to content

Error on recursive opaque ty in HIR typeck #139419

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Apr 5, 2025

"Non-trivially recursive" opaques are opaques whose hidden types are inferred to be equal to something other than themselves. For example, if we have a TAIT like type TAIT = impl Sized, if we infer the hidden type to be TAIT := (TAIT,), that would be a non-trivial recursive definition. We don't want to support opaques that are non-trivially recursive, since they will (almost!! -- see caveat below) always result in borrowck errors, and are generally a pain to deal with.

On the contrary, trivially recursive opaques may occur today because the old solver overagerly uses replace_opaque_types_with_inference_vars. This infer var can then later be constrained to be equal to the opaque itself. These cases will not necessarily result in borrow-checker errors, since other uses of the opaque may properly constrain the opaque. If there are no other uses we may instead fall back to () today.

The only weird case that we have to unfortunately deal with was discovered in #139406:

#![allow(unconditional_recursion)]

fn what1<T>(x: T) -> impl Sized {
    what1(x)
}

fn what2<T>(x: T) -> impl Sized {
    what2(what2(x))
}

fn print_return_type<T, U>(_: impl Fn(T) -> U) {
    println!("{}", std::any::type_name::<U>())
}

fn main() {
    print_return_type(what1::<i32>); // ()
    print_return_type(what2::<i32>); // i32
}

HIR typeck eagerly replaces the return type with an infer var, ending up with RPIT<T> = RPIT<RPIT<T>> in the storage. While we return this in the TypeckResults, it's never actually used anywhere.

MIR building then results in the following statement

let _0: impl RPIT<T> /* the return place */ = build<RPIT<T>>(_some_local);

Unlike HIR typeck MIR typeck now directly equates RPIT<T> with RPIT<RPIT<T>>. This does not try to define RPIT but instead relates its generic arguments

(
&ty::Alias(ty::Opaque, ty::AliasTy { def_id: a_def_id, .. }),
&ty::Alias(ty::Opaque, ty::AliasTy { def_id: b_def_id, .. }),
) if a_def_id == b_def_id => {
super_combine_tys(infcx, self, a, b)?;
}

This means we relate T with RPIT<T>, which results in a defining use RPIT<T> = T

I think it's pretty obvious that this is not desirable behavior, and according to the crater run there were no regressions, so let's break this so that we don't have any inference hazards in the new solver.

In the future what2 may end up compiling again by also falling back to (). However, that is not yet guaranteed and the transition to this state is made significantly harder by not temporarily breaking it on the way. It is also concerning to change the inferred hidden type like this without any notification to the user, even if likely not an issue in this concrete case.

@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 5, 2025
@compiler-errors
Copy link
Member Author

compiler-errors commented Apr 5, 2025

@bors try

@rust-timer

This comment was marked as off-topic.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2025
…<try>

Just error on recursive opaque ty in HIR typeck

Opening just to gauge fallout.

r? `@ghost`
@bors
Copy link
Collaborator

bors commented Apr 5, 2025

⌛ Trying commit 99e7ce9 with merge f45c1ab...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 5, 2025

☀️ Try build successful - checks-actions
Build commit: f45c1ab (f45c1ab62d4de6b63f69b4daa9d1f0b90c38a208)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f45c1ab): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -2.2%)

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)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 775.534s -> 775.663s (0.02%)
Artifact size: 365.93 MiB -> 365.92 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2025
@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-139419 created and queued.
🤖 Automatically detected try build f45c1ab
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-139419 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-139419 is completed!
📊 3 regressed and 2 fixed (610052 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 7, 2025
@lcnr
Copy link
Contributor

lcnr commented Apr 7, 2025

all spurious, can you think of a test case which actually compiled before this PR?

I would expect us to error for them already, just after rustc_borrowck

@lcnr
Copy link
Contributor

lcnr commented Apr 7, 2025

This is a breaking change, see #139406. We should very much break this, explaining what's going on in that issue

@compiler-errors compiler-errors changed the title Just error on recursive opaque ty in HIR typeck Error on recursive opaque ty in HIR typeck Apr 16, 2025
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-19 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#18 exporting to docker image format
#18 sending tarball 20.1s done
#18 DONE 27.3s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-19]
[CI_JOB_NAME=x86_64-gnu-llvm-19]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Listening on address 127.0.0.1:4226
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-19', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'build.print-step-timings', '--enable-verbose-tests', '--set', 'build.metrics', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--set', 'gcc.download-ci-gcc=true', '--enable-new-symbol-mangling']
configure: build.build          := x86_64-unknown-linux-gnu
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-19/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
[RUSTC-TIMING] proc_macro test:false 5.855
[RUSTC-TIMING] test test:false 9.202
    Finished `release` profile [optimized] target(s) in 1m 17s
##[endgroup]
[TIMING] core::build_steps::compile::Std { target: x86_64-unknown-linux-gnu, compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu, forced_compiler: true }, crates: [], force_recompile: false, extra_rust_args: ["-Csymbol-mangling-version=v0", "-Cpanic=abort"], is_for_mir_opt_tests: false } -- 77.726
Testing GCC stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
 Downloading crates ...
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
[RUSTC-TIMING] boml test:false 0.779
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
[RUSTC-TIMING] y test:false 2.948
    Finished `release` profile [optimized] target(s) in 4.25s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-backend gcc --gcc-path /checkout/obj/build/x86_64-unknown-linux-gnu/ci-gcc/lib --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
`--gcc-path` was provided, ignoring config file. Using `/checkout/obj/build/x86_64-unknown-linux-gnu/ci-gcc/lib` as path for libgccjit
[BUILD] mini_core
[RUSTC-TIMING] mini_core test:false 0.202
[BUILD] example
[AOT] mini_core_hello_world
[RUSTC-TIMING] mini_core_hello_world test:false 0.180
---
  = note: `#[warn(unconditional_recursion)]` on by default

error: aborting due to 1 previous error; 1 warning emitted

Some expected error codes were not found: ["E0720"]

failures:
    /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0720 (line 15607)

test result: FAILED. 1046 passed; 1 failed; 61 ignored; 0 measured; 0 filtered out; finished in 9.56s

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:39:41
  local time: Mon Apr 28 20:41:01 UTC 2025
  network time: Mon, 28 Apr 2025 20:41:01 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@compiler-errors compiler-errors marked this pull request as ready for review April 28, 2025 20:45
@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2025

Some changes occurred in exhaustiveness checking

cc @Nadrieril

@compiler-errors compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 28, 2025
@compiler-errors
Copy link
Member Author

I mentioned in the description but I'll say it again -- no crater regressions AFAICT.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 28, 2025

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 28, 2025
@lcnr
Copy link
Contributor

lcnr commented Apr 29, 2025

#139587 currently does not support the code disallowed by this FCP, so I am very much in favor. The only sensible way to support this relies on properly normalizing opaque types in HIR typeck which is sadly still some way off and relies on #139587. With this

@rfcbot reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants