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

Get rid of check_opaque_type_well_formed #132757

Merged
merged 2 commits into from
Nov 9, 2024

Conversation

compiler-errors
Copy link
Member

Instead, replicate it by improving the span of the opaque in check_opaque_meets_bounds.

This has two consequences:

  1. We now prefer "concrete type differs" errors, since we'll hit those first before we check the opaque is WF.
  2. Spans have gotten slightly worse.

Specifically, (2.) could be improved by adding a new obligation cause that explains that the definition's environment has stronger assumptions than the declaration.

r? lcnr

@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 Nov 8, 2024

fn define<'a, 'b>() -> Opaque<'a, 'b>
where
'a: 'b,
{
|| {}
//~^ ERROR lifetime bound not satisfied
Copy link
Member Author

Choose a reason for hiding this comment

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

This is as bad as outlives errors we get within a defining scope, like:

fn foo<'a, T>(t: T) -> impl Sized + 'a { t }
error[E0309]: the parameter type `T` may not live long enough
 --> src/lib.rs:1:42
  |
1 | fn foo<'a, T>(t: T) -> impl Sized + 'a { t }
  |        --                                ^ ...so that the type `T` will meet its required lifetime bounds
  |        |
  |        the parameter type `T` must be valid for the lifetime `'a` as defined here...

Copy link
Contributor

Choose a reason for hiding this comment

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

can we also emit the "this definition site has more where clauses than the opaque type" note for region errors, I feel like it's even more useful there

Copy link
Member Author

Choose a reason for hiding this comment

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

really annoying to do that given the way that region error reporting is handled with a totally different error path :/

@@ -364,6 +393,97 @@ fn check_opaque_meets_bounds<'tcx>(
}
}

fn best_definition_site_of_opaque<'tcx>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kinda annoyingly duplicated w/ the TAIT visitor, but also I could not find a way to genericize that without making it a mess, since it has a lot of responsibilities.

@lcnr
Copy link
Contributor

lcnr commented Nov 8, 2024

cc @oli-obk

r=me after suggestion

@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2024

I remember we added the duplicate check in borrowck because the wf check failed to handle lifetimes in all cases.

@compiler-errors
Copy link
Member Author

@oli-obk: #96736 does not seem to suggest that this extra well-formedness check is necessary for correctness, and instead is used for improving diagnostics.

I remember we added the duplicate check in borrowck because the wf check failed to handle lifetimes in all cases.

We don't even check lifetimes in the borrowck copy of the well-formedness check, so I'm not totally sure what you mean.... As far as I can tell, the checks in check_opaque_meets_bounds are a strict subset of what we're checking in check_opaque_type_well_formed.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2024

Ah, I misremembered. Thanks for investigating.

@compiler-errors
Copy link
Member Author

I spent an hour on this and could not get the lifetime error reporting machinery to work correctly, so I won't do anything else to this PR.

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Nov 8, 2024

📌 Commit 97dfe8b has been approved by lcnr

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 Nov 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
Get rid of `check_opaque_type_well_formed`

Instead, replicate it by improving the span of the opaque in `check_opaque_meets_bounds`.

This has two consequences:
1. We now prefer "concrete type differs" errors, since we'll hit those first before we check the opaque is WF.
2. Spans have gotten slightly worse.

Specifically, (2.) could be improved by adding a new obligation cause that explains that the definition's environment has stronger assumptions than the declaration.

r? lcnr
@bors
Copy link
Contributor

bors commented Nov 8, 2024

⌛ Testing commit 97dfe8b with merge 2a120f8...

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
---- [run-make] tests\run-make\avr-rjmp-offset stdout ----

error: rmake recipe failed to complete
status: exit code: 1
command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake.exe"
--- stderr -------------------------------
command failed at line 29
command failed at line 29
Command { cmd: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\bin\\rustc.exe" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake_out" "avr-rjmp-offsets.rs" "-Copt-level=s" "-Cpanic=abort" "--target=avr-unknown-gnu-atmega328" "-Clinker=rust-lld" "-Clink-arg=--entry=main" "-o" "compiled", stdin_buf: None, stdin: None, stdout: None, stderr: None, drop_bomb: DropBomb { command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\bin\\rustc.exe", defused: true, armed_location: Location { file: "C:\\a\\rust\\rust\\tests\\run-make\\avr-rjmp-offset\\rmake.rs", line: 16, col: 5 } }, already_executed: true }
output status: `exit code: 1`
=== STDOUT ===


=== STDERR ===
error: linking with `rust-lld` failed: exit code: 0xc0000005
error: linking with `rust-lld` failed: exit code: 0xc0000005
  |
  = note: "rust-lld" "-flavor" "gnu" "C:\\a\\_temp\\msys64\\tmp\\rustcHzwA2Q\\symbols.o" "compiled.avr_rjmp_offsets.ab053966543a1f9f-cgu.0.rcgu.o" "--as-needed" "-Bdynamic" "-z" "noexecstack" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake_out" "-o" "compiled" "--gc-sections" "--entry=main"
  = note: PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.␍
          Stack dump:␍
          0.    Program arguments: rust-lld -flavor gnu C:\\a\\_temp\\msys64\\tmp\\rustcHzwA2Q\\symbols.o compiled.avr_rjmp_offsets.ab053966543a1f9f-cgu.0.rcgu.o --as-needed -Bdynamic -z noexecstack -L C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake_out -o compiled --gc-sections --entry=main␍
          Exception Code: 0xC0000005␍
          0x00007FFB23E97A05, C:\Windows\SYSTEM32\ntdll.dll(0x00007FFB23E80000) + 0x17A05 byte(s), RtlEnterCriticalSection() + 0x585 byte(s)␍
          0x00007FFB23E9B550, C:\Windows\SYSTEM32\ntdll.dll(0x00007FFB23E80000) + 0x1B550 byte(s), RtlGetCurrentServiceSessionId() + 0xBF0 byte(s)␍
          0x00007FFB23E9A8C1, C:\Windows\SYSTEM32\ntdll.dll(0x00007FFB23E80000) + 0x1A8C1 byte(s), RtlFreeHeap() + 0x51 byte(s)␍
          0x00007FFB2299C69C, C:\Windows\System32\msvcrt.dll(0x00007FFB22980000) + 0x1C69C byte(s), free() + 0x1C byte(s)␍
          0x00007FFB2299BD46, C:\Windows\System32\msvcrt.dll(0x00007FFB22980000) + 0x1BD46 byte(s), _aligned_free() + 0x16 byte(s)␍
          0x00007FF737072F1A, C:\a\rust\rust\build\x86_64-pc-windows-gnu\stage2\lib\rustlib\x86_64-pc-windows-gnu\bin\rust-lld.exe(0x00007FF732D30000) + 0x4342F1A byte(s)
error: aborting due to 1 previous error
------------------------------------------


@bors
Copy link
Contributor

bors commented Nov 8, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2024
@compiler-errors
Copy link
Member Author

lol that definitely has nothing to do with opaques

@bors retry

@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 Nov 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#132755 (Do not reveal opaques in the param-env, we got lazy norm instead)
 - rust-lang#132757 (Get rid of `check_opaque_type_well_formed`)
 - rust-lang#132760 (Don't suggest `.into_iter()` on iterators)
 - rust-lang#132778 (update io::Error::into_inner to acknowledge io::Error::other)
 - rust-lang#132780 (use verbose for path separator suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7a49704 into rust-lang:master Nov 9, 2024
6 of 7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
Rollup merge of rust-lang#132757 - compiler-errors:yeet-check-wf, r=lcnr

Get rid of `check_opaque_type_well_formed`

Instead, replicate it by improving the span of the opaque in `check_opaque_meets_bounds`.

This has two consequences:
1. We now prefer "concrete type differs" errors, since we'll hit those first before we check the opaque is WF.
2. Spans have gotten slightly worse.

Specifically, (2.) could be improved by adding a new obligation cause that explains that the definition's environment has stronger assumptions than the declaration.

r? lcnr
@compiler-errors compiler-errors deleted the yeet-check-wf branch November 9, 2024 11:41
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Get rid of `check_opaque_type_well_formed`

Instead, replicate it by improving the span of the opaque in `check_opaque_meets_bounds`.

This has two consequences:
1. We now prefer "concrete type differs" errors, since we'll hit those first before we check the opaque is WF.
2. Spans have gotten slightly worse.

Specifically, (2.) could be improved by adding a new obligation cause that explains that the definition's environment has stronger assumptions than the declaration.

r? lcnr
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#132755 (Do not reveal opaques in the param-env, we got lazy norm instead)
 - rust-lang#132757 (Get rid of `check_opaque_type_well_formed`)
 - rust-lang#132760 (Don't suggest `.into_iter()` on iterators)
 - rust-lang#132778 (update io::Error::into_inner to acknowledge io::Error::other)
 - rust-lang#132780 (use verbose for path separator suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
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