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

[WIP] Add const qualifier in FnSig (for const fn pointers) #74553

Closed
wants to merge 15 commits into from

Conversation

filtsin
Copy link

@filtsin filtsin commented Jul 20, 2020

Impl #63997

Add const qualifier in FnSig like unsafe or extern.

With this changes we can write something like this:

const fn foo() {}
const FOO: const fn() = foo;
const fn bar() { FOO() }
const fn baz(x: const fn()) { x() }
const fn bazz() { baz(FOO) }

or we can work around of const traits:

trait Bar { const F: const fn(Self) -> Self; }

const fn map_i32(x: i32) -> i32 { x * 2 }
impl Bar for i32 { const F: const fn(Self) -> Self = map_i32; } 
const fn map_u32(x: u32) -> u32 { x * 3 }
impl Bar for u32 { const F: const fn(Self) -> Self = map_u32; } 

const fn foo(f: fn()) {
  f(); // <- is not allowed becase fn() is not const fn()
}
const fn foo(f: const fn()) {
  f(); // allow with this pr
}
fn foo(f: const fn()) {
  f(); // allow 
}

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2020
@filtsin filtsin changed the title [WIP] Add const qualifier to fn header (for const fn pointers) [WIP] Add const qualifier in FnSig (for const fn pointers) Jul 20, 2020
@filtsin filtsin marked this pull request as draft July 20, 2020 18:00
src/librustc_ast_passes/ast_validation.rs Outdated Show resolved Hide resolved
a: ast::Constness,
b: ast::Constness,
) -> RelateResult<'tcx, ast::Constness> {
if a == b { Ok(a) } else { Ok(ast::Constness::NotConst) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be if a == b { Ok(a) } else { Err(...) }, coercing from const fn() to fn() and from a const function item to a normal function pointer should be handled by adding a new variant to PointerCast in src/librustc_middle/ty/adjustment.rssrc/librustc_middle/ty/adjustment.rs and using it as appropriate in src/librustc_typeck/check/coercion.rs.

Copy link
Author

@filtsin filtsin Jul 21, 2020

Choose a reason for hiding this comment

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

I added 2 types of pointer casting: NotConstFnPointer (const fn() => fn()), UnsafeNotConstFnPointer (const fn() => unsafe fn()) becase I can not find a better solution. But I think that it is not a good.

src/librustc_parse/parser/ty.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jul 21, 2020

☔ The latest upstream changes (presumably #69749) made this pull request unmergeable. Please resolve the merge conflicts.

@filtsin
Copy link
Author

filtsin commented Jul 21, 2020

Can I ask? What's the best way for implementation of implicit casting?

Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

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

You should be able to chain an Unsafe and a NotConst coercion when you would use UnsafeNotConstFnPointer.

src/librustc_codegen_ssa/mir/rvalue.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/check_consts/validation.rs Outdated Show resolved Hide resolved
@filtsin
Copy link
Author

filtsin commented Jul 28, 2020

I try to add implicit casting for drop const qualifier:

const fn foo() { }
fn bar() { }

fn main() {
 let x = if true { bar } else { foo };
}

What's the best way for implementing this feature? How can I do ReifyFnPointer to not const fn pointer in this situation?

@filtsin
Copy link
Author

filtsin commented Jul 30, 2020

I added an example of how can I do implicit cast for drop const qualificator. Is it good?

Now I need edit check_match for this situation:

const fn foo() {}
const fn bar() {}
fn baz() {}

fn main() {
 let x = match 2 {
  2 => foo,
  3 => bar,
  _ => baz
 };
}

@bors
Copy link
Contributor

bors commented Aug 8, 2020

☔ The latest upstream changes (presumably #74877) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Aug 15, 2020

☔ The latest upstream changes (presumably #73851) made this pull request unmergeable. Please resolve the merge conflicts.

@filtsin filtsin marked this pull request as ready for review August 21, 2020 09:12
@bors
Copy link
Contributor

bors commented Aug 22, 2020

☔ The latest upstream changes (presumably #75797) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2020
@crlf0710
Copy link
Member

@filtsin Ping from triage, any updates on this?

@filtsin
Copy link
Author

filtsin commented Sep 18, 2020

I've no idea how can I resolve this question #74553 (comment)

Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

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

I've left some comments for how to get cases like that working correctly.

@@ -925,7 +984,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// We have a LUB of prev_ty and new_ty, just return it.
Copy link
Contributor

Choose a reason for hiding this comment

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

The patterns in the match on (&prev_ty.kind, &new_ty.kind) should use &ty::FnDef(..) | &ty::FnPtr(..) instead of &ty::FnDef(..)

} else {
Adjust::Pointer(PointerCast::ReifyFnPointer)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This match and the next would be changed to return an Option and an arm for ty::FnPtr(..) should be added here. Some(Adjust::Pointer(PointerCast::NotConstFnPointer)) would be returned if prev_drop_const, otherwise None would be returned. The apply_adjustments calls would be skipped if None is returned from the match.

apply_adjustments would need to be changed to handle the case where a NotConstFnPointer adjustment is applied when there is already a ReifyFnPointer adjustment (by merging them to a NotConstFnPointer adjustment).

@filtsin filtsin force-pushed the master branch 3 times, most recently from e90d74c to 0b0be68 Compare October 12, 2020 06:29
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2020
@bors
Copy link
Contributor

bors commented Oct 14, 2020

☔ The latest upstream changes (presumably #77135) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Oct 14, 2020

☔ The latest upstream changes (presumably #77926) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Oct 17, 2020

☔ The latest upstream changes (presumably #77685) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@matthewjasper
Copy link
Contributor

cc @rust-lang/wg-const-eval

ty::FnPtr(_) => {
// No change to value
self.write_immediate(*src, dest)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please merge these two new match arms into the existing one for UnsafeFnPointer... seems unnecessary to duplicate that code.

ty::FnPtr(fn_sig) => {
// At this point, we are calling a function by raw pointer because
// we know that it is const
if self.ccx.tcx.features().const_fn_pointer
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with "by raw pointer"? A raw pointer would be *const T/*mut T, and I do not think there is such a pointer here.

const _: const unsafe fn() = unsafe_fn;
//~^ ERROR mismatched types

const _: unsafe fn() = const_unsafe_fn;
Copy link
Member

Choose a reason for hiding this comment

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

The file is called "const_fn_ptr_cast" but it only seems to check coercions, not casts (via as). Is that the same code in the compiler or should as-casts be tested explicitly as well?

@crlf0710
Copy link
Member

@filtsin Ping from triage, any updates on this?

@Dylan-DPC-zz
Copy link

@filtsin thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants