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

NLL: spurious error emitted on body of (erroneous) const fn body #55825

Closed
pnkfelix opened this issue Nov 9, 2018 · 3 comments · Fixed by #89505
Closed

NLL: spurious error emitted on body of (erroneous) const fn body #55825

pnkfelix opened this issue Nov 9, 2018 · 3 comments · Fixed by #89505
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-diagnostics Working torwads the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Nov 9, 2018

Spawned off of #54528 (specificially this card in the project).

Consider the following code:

const fn no_dyn_trait_ret() -> &'static dyn std::fmt::Debug { &() }

fn main() { }

With AST-borrowck, you get (play):

error: trait bounds other than `Sized` on const fn parameters are unstable
 --> src/main.rs:1:32
  |
1 | const fn no_dyn_trait_ret() -> &'static dyn std::fmt::Debug { &() }
  |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

With NLL (here using migration a la 2018 edition), you get (play):

error: trait bounds other than `Sized` on const fn parameters are unstable
 --> src/main.rs:1:32
  |
1 | const fn no_dyn_trait_ret() -> &'static dyn std::fmt::Debug { &() }
  |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning[E0515]: cannot return reference to temporary value
 --> src/main.rs:1:63
  |
1 | const fn no_dyn_trait_ret() -> &'static dyn std::fmt::Debug { &() }
  |                                                               ^--
  |                                                               ||
  |                                                               |temporary value created here
  |                                                               returns a reference to data owned by the current function
  |
  = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
          It represents potential unsoundness in your code.
          This warning will become a hard error in the future.

error: aborting due to previous error

I have not managed to replicate the above borrow-check error on code that does not exhibit some other const fn error.

And the problem goes away if you opt into #![feature(const_fn)] (play).

So, this is really a very minor diagnostic issue.

@RalfJung
Copy link
Member

Cc @oli-obk might be something in (min) const qualification?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 15, 2018

TLDR: The issue with this is that we have a bunch of separate checks relating to const things, and most of these are spaghetti entangled into one. @eddyb and I are working towards fixing that, but it's nontrivial to say the least.

Details:

We have the following checks that we need to run on MIR (which relate to const):

  • min_const_fn compliance
  • const_fn compliance
  • promotion of runtime values to an anonymous static/constant
  • const compliance (valid array length, const item initializer or enum discriminant)
  • static compliance (valid static item initializer)
  • no interior mutability checks
  • no drop in const checks

The current implementation has an early abort for const fns where if the const_fn feature gate is not active, we first run min_const_fn, and if that fails we don't run the entangled other 4 passes. If we ran the other passes in case of min_const_fn failure, you'd get a lot of error duplication.

I don't think we have a tracking issue for the refactoring separating all these passes into distinct dataflow passes, but there's ongoing work.

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. NLL-diagnostics Working torwads the "diagnostic parity" goal labels Jan 19, 2019
@crlf0710 crlf0710 added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2020
@Aaron1011
Copy link
Member

This now produces the same output both with and without #![feature(nll)].

@Aaron1011 Aaron1011 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 2, 2021
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…mulacrum

Add regression test for spurious const error with NLL

Fixes rust-lang#55825
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…mulacrum

Add regression test for spurious const error with NLL

Fixes rust-lang#55825
@bors bors closed this as completed in 5b66048 Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-diagnostics Working torwads the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants