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

Function pointers break in const code after ~const bounds are added #109543

Closed
cuviper opened this issue Mar 23, 2023 · 5 comments · Fixed by #109557
Closed

Function pointers break in const code after ~const bounds are added #109543

cuviper opened this issue Mar 23, 2023 · 5 comments · Fixed by #109557
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper commented Mar 23, 2023

Code

I first noticed this in #109122 (review), but there are also existing regressions from other ~const additions. For example:

type I32Cmp = fn(&i32, &i32) -> core::cmp::Ordering;
pub const fn min_by_i32() -> fn(i32, i32, I32Cmp) -> i32 {
    core::cmp::min_by
}

I expected to see this happen: compiles fine.

Instead, this happened: errors that ~const FnOnce is not implemented for the I32Cmp function pointer.

error[E0277]: the trait bound `for<'a, 'b> for<'a, 'b> fn(&'a i32, &'b i32) -> std::cmp::Ordering: FnOnce<(&'a i32, &'b i32)>` is not satisfied
 --> error.rs:3:5
  |
3 |     core::cmp::min_by
  |     ^^^^^^^^^^^^^^^^^ expected an `FnOnce<(&i32, &i32)>` closure, found `for<'a, 'b> fn(&'a i32, &'b i32) -> std::cmp::Ordering`
  |
  = help: the trait `for<'a, 'b> ~const FnOnce<(&'a i32, &'b i32)>` is not implemented for `for<'a, 'b> fn(&'a i32, &'b i32) -> std::cmp::Ordering`
note: the trait `for<'a, 'b> FnOnce<(&'a i32, &'b i32)>` is implemented for `for<'a, 'b> fn(&'a i32, &'b i32) -> std::cmp::Ordering`, but that implementation is not `const`
 --> error.rs:3:5
  |
3 |     core::cmp::min_by
  |     ^^^^^^^^^^^^^^^^^
note: required by a bound in `std::cmp::min_by`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

Version it worked on

For that particular example, it most recently worked on: 1.65.0

Version with regression

After #102245 reached stable, rustc --version --verbose:

rustc 1.66.0 (69f9c33d7 2022-12-12)
binary: rustc
commit-hash: 69f9c33d71c871fc16ac445211281c6e7a340943
commit-date: 2022-12-12
host: x86_64-unknown-linux-gnu
release: 1.66.0
LLVM version: 15.0.2

... but I don't mean to pick on that particular PR -- I think there's a more general problem with ~const here.

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

@cuviper cuviper added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Mar 23, 2023
@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed regression-untriaged Untriaged performance or correctness regression. labels Mar 23, 2023
@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Mar 23, 2023
@Mark-Simulacrum
Copy link
Member

Nominating for libs-api - though may want to hold discussion until there's a const folks opinion on how hard a fix will be in the compiler. The code is pretty esoteric but still seems bad to break.

@cuviper did this appear in real world code? Or just happened to stumble across it?

@thomcc thomcc added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Mar 23, 2023
@cuviper
Copy link
Member Author

cuviper commented Mar 23, 2023

It appeared as a new error in tests/ui/consts/issue-102117.rs, where it suddenly wanted const-droppable T for the new ~const bounds on drop_in_place in #109122. That case actually seems fairly realistic for manual vtables, but I haven't seen any other errors like this in the wild.

@thomcc
Copy link
Member

thomcc commented Mar 23, 2023

CC @rust-lang/wg-const-eval

@fee1-dead
Copy link
Member

I'm going to take a look on resolving this in the compiler.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 27, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 28, 2023
…=oli-obk

Move const trait bounds checks to MIR constck

Fixes rust-lang#109543. When checking paths in HIR typeck, we don't want to check for const predicates since all we want might just be a function pointer. Therefore we move this to MIR constck and check that bounds are met during MIR constck.

r? `@oli-obk`
@bors bors closed this as completed in b17e668 Mar 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 31, 2023
Revert 102245

reverts rust-lang#102245, but only on beta, as that caused rust-lang#109543 which has since been fixed on nightly.

This needed an additional partial revert of rust-lang@5b08c9f for changes that happened due to later cleanups.

I also added a test from the issue to make sure this actually fixes it 😆

r? `@wesleywiser` `@pnkfelix`
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants