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

use correct feature flag for impl-block-level trait bounds on const fn #84556

Merged
merged 3 commits into from Apr 29, 2021

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 25, 2021

I am not sure what that special hack was needed for, but it doesn't seem needed any more...

This removes the last use of the const_fn feature flag -- Cc #84510
r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2021
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

Maybe we don't have tests for the situation the comment was referring to? Check if we have a test where a generic impl const block can use its trait bounds in a function, if not, please add one

@RalfJung
Copy link
Member Author

RalfJung commented Apr 25, 2021 via email

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

On phone, but i'll try:

impl<T: Foo> const Bar for () {
    fn bar() {
        T::foo()
    } 
}

Declare the Foo and Bar traits appropriately.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 26, 2021

I did that, but I am not sure if that is the test the comment was referring to... the comment sounded more like a situation where a const Trait bound somewhere was incorrectly not recognized as const and hence, I assume, leads to an error -- IOW, the comment referred to some code incorrectly failing to build (not some code incorrectly building).

impl<T: const Plus> const Plus for Num<T> does not even parse, though.
This code does parse and, to my surprise, fails to build:

#![feature(const_fn_trait_bounds)]
#![feature(const_trait_impl)]
#![feature(const_trait_bound_opt_out)]
#![allow(incomplete_features)]

mod num {
    pub trait Plus {
        fn plus(self, rhs: Self) -> Self;
    }

    struct Num<T>(T);
    impl<T: ?const Plus + Copy> const Plus for Num<T> {
        fn plus(self, rhs: Self) -> Self {
            Num(self.0.plus(rhs.0))
        }
    }
}

However, the error has nothing to do with feature gating:

error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
  --> /home/r/src/rust/rustc.2/src/test/ui/rfc-2632-const-trait-impl/call-generic-method-nonconst-opt-out.rs:30:17
   |
LL |             Num(self.0.plus(rhs.0))
   |                 ^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

So, I don't think what this PR does makes things any worse here.

@RalfJung
Copy link
Member Author

I think what is left here is to fix lock_api -- I'll submit a PR there tomorrow (once #84578 is in nightly).

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 26, 2021

This code does parse and, to my surprise, fails to build:

Why are you surprised it fails to build? It should fail, you are using ?const to opt out of constness after all

@oli-obk
Copy link
Contributor

oli-obk commented Apr 26, 2021

The problem is that it also fails to compile without ?const

@RalfJung
Copy link
Member Author

Why are you surprised it fails to build? It should fail, you are using ?const to opt out of constness after all

Ah, I forgot that things are implicitly const.^^

@RalfJung
Copy link
Member Author

The problem is that it also fails to compile without ?const

Okay, I guess I should remove my new test then? But it says nothing about a feature gate there, so I don't think my PR changes the situation here... it might mean some people need to add feature(const_fn_trait_bounds) but that does not seem unreasonable?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 26, 2021

The problem is that it also fails to compile without ?const

Okay, I guess I should remove my new test then? But it says nothing about a feature gate there, so I don't think my PR changes the situation here... it might mean some people need to add feature(const_fn_trait_bounds) but that does not seem unreasonable?

we already have a test for this behaviour and possibly even an issue, idk where though

bors bot added a commit to Amanieu/parking_lot that referenced this pull request Apr 26, 2021
281: add new const_fn_trait_bound feature gate r=Amanieu a=RalfJung

This (and a subsequent release of `lock_api`) is needed to make rust-lang/rust#84556 work.

Co-authored-by: Ralf Jung <post@ralfj.de>
@RalfJung
Copy link
Member Author

we already have a test for this behaviour and possibly even an issue, idk where though

Okay so I removed the new test again. Should I add anything else instead?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 26, 2021

we already have a test for this behaviour and possibly even an issue, idk where though

Okay so I removed the new test again. Should I add anything else instead?

idk, I don't have the time to really dig into this, so let's just merge as is. It preserves the status quo, so all is good

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

Cc Amanieu/parking_lot#281

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 29, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2021

📌 Commit 3c4c5eb has been approved by oli-obk

@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 Apr 29, 2021
@bors
Copy link
Contributor

bors commented Apr 29, 2021

⌛ Testing commit 3c4c5eb with merge 18587b1...

@bors
Copy link
Contributor

bors commented Apr 29, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 18587b1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2021
@bors bors merged commit 18587b1 into rust-lang:master Apr 29, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 29, 2021
@RalfJung RalfJung deleted the const-fn-trait-bound branch May 1, 2021 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants