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 pointer signature containing &mut T should work in const context #114994

Closed
nayeemrmn opened this issue Aug 19, 2023 · 4 comments · Fixed by #116015
Closed

Function pointer signature containing &mut T should work in const context #114994

nayeemrmn opened this issue Aug 19, 2023 · 4 comments · Fixed by #116015
Assignees
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug.

Comments

@nayeemrmn
Copy link

nayeemrmn commented Aug 19, 2023

Proposing to extract this from #57349.

The following examples shouldn't error:

const fn get_some_fn() -> fn(&mut String) {
  String::clear
}

const fn use_const_fn(_f: fn(&mut String)) {
  ()
}

const fn some_const_fn() {
  let _f: fn(&mut String) = String::clear;
}

// mutable references are not allowed in constant functions      x3

While this has been considered part of #57349 for a while, it's also been acknowledged as a bug and suggested by @RalfJung to address separately ahead of stabilizing const_mut_refs. Indeed it's surprising for users that this would count as handling a mutable reference in const contexts.

See:
#57349 (comment)
#57349 (comment)

@nayeemrmn nayeemrmn added the C-bug Category: This is a bug. label Aug 19, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 19, 2023
@RalfJung
Copy link
Member

Cc @rust-lang/wg-const-eval

Fixing this requires changing this logic to skip traversing things inside function pointers. I'm not sure what the best way to achieve that is.

@RalfJung RalfJung added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Aug 19, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 19, 2023
@EvanMerlock
Copy link
Contributor

@rustbot claim

EvanMerlock added a commit to EvanMerlock/rust that referenced this issue Sep 21, 2023
EvanMerlock added a commit to EvanMerlock/rust that referenced this issue Sep 23, 2023
- add new testcase for TypeVisitor on const-eval mutable ref check
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 14, 2023
const_eval: allow function pointer signatures containing &mut T in const contexts

potentially fixes rust-lang#114994

We utilize a `TypeVisitor` here in order to more easily handle control flow.
- In the event the typekind the Visitor sees is a function pointer, we skip over it
- However, otherwise we do one of two things:
   - If we find a mutable reference, check it, then continue visiting types
   - If we find any other type, continue visiting types

This means we will check if the function pointer _itself_ is mutable, but not if any of the types _within_ are.
@bors bors closed this as completed in 139f63a Oct 14, 2023
@kaffarell
Copy link
Contributor

Is there any reason returning a function trait object (e.g. dyn Fn, dyn FnOnce, etc.) does not work? It works with the const_mut_refs feature, but IMO it should have been pulled out like the function pointers. For example this does not compile:

fn something(_s: &mut String) {
}

const fn unwrap_cool() -> &'static dyn Fn(&mut String) {
    &something
}

@RalfJung
Copy link
Member

We didn't exhaustively look through all cases that we can exempt here.

Anyway, const_mut_refs is on the path to stabilization, so this question will be moot soon. :)

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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants