More DefineOpaqueTypes::Yes #123794

oli-obk · 2024-04-11T14:30:35Z

This accepts more code on stable. It is now possible to have match arms return a function item foo::<ConcreteType> and a function item foo::<OpaqueTypeInDefiningScope> in another, and that will constrain OpaqueTypeInDefiningScope to have the hidden type ConcreteType. So the following function will now compile, but on master it errors with a type mismatch on the second match arm

// The function item whose generic params we want to merge.
fn foo<T>(t: T) -> T { t }
// Helper ensuring we can constrain `T` on `F` without explicitly specifying it
fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f }

fn k() -> impl Sized {
    let x = match true {
        true => {
            // `f` is `FnDef(foo, [infer_var])`
            let f = foo;
            // Get a value of an opaque type on stable
            let t = k();
            // this returns `FnDef(foo, [k::return])`
            bind(t, f)
        }
        false => foo::<()>,
    };
    todo!()
}

r? @compiler-errors

cc #116652

tests/ui/fn/fn_def_coercion.rs

oli-obk · 2024-04-12T10:49:11Z

@rfcbot merge

See main post for an explanation of the new behaviour. Imo this is the expected behaviour, especially as we already do this for other types:

fn k() -> impl Sized {
    let x = match true {
        true => {
            // Get a value of an opaque type on stable
            let t = k();
            (t,)
        }
        false => ((),),
    };
    todo!()
}

rfcbot · 2024-04-12T10:49:13Z

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

jackh726 · 2024-04-12T12:10:18Z

Is this just for consistency? Or was there some motivating example for this?

I worry about continuing to expand the scope of what opaques can do, while we're currently trying to stabilise more of them (associated type impl trait). Maybe this is too pessimistic of a view, because this does seen fairly straightforward, but I'd hate to see us accidentally introduce another bug or subtlety that we have to consider.

Also, I think at the very least @rust-lang/lang should be pinged on this, even if not part of the FCP.

lcnr · 2024-04-12T13:28:53Z

compiler/rustc_hir_typeck/src/coercion.rs

@@ -1145,7 +1145,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                        // are the same function and their parameters have a LUB.
                        match self.commit_if_ok(|_| {
                            self.at(cause, self.param_env).lub(
-                                DefineOpaqueTypes::No,
+                                DefineOpaqueTypes::Yes,


DefineOpaqueTypes::Yes inside of a commit_if_ok likely means that this can cause type inference breakage.

When coercing function definitions we may have previously used the lub further down as this lub here failed. This would have then caused us to return a function pointer (i think), whereas we now return an FnDef.

I would like to see a crater run here to allow us to more eagerly detect any regressions from it

lcnr · 2024-04-12T13:36:40Z

Is this just for consistency? Or was there some motivating example for this?

consistency with -Znext-solver 😊 the new solver does not have the concept of "non-defining use of opaque" right now and I would like to ideally keep it that way. I consider moving to DefineOpaqueTypes::Yes in more cases to remove subtlety from the type system. RIght now we have to be careful when relating Opaque with another type as the behavior changes depending on whether we later use the Opaque or its hidden type directly (even though they are equal) edit: if that later use is with DefineOpaqueTypes::No

jackh726 · 2024-04-12T13:58:16Z

consistency with -Znext-solver 😊

👍 perfect

oli-obk · 2024-04-12T14:23:53Z

@bors try

bors · 2024-04-12T14:25:05Z

⌛ Trying commit 4abbda6 with merge f0a46ff...

More DefineOpaqueTypes::Yes This accepts more code on stable. It is now possible to have match arms return a function item `foo::<ConcreteType>` and a function item `foo::<OpaqueTypeInDefiningScope>` in another, and that will constrain `OpaqueTypeInDefiningScope` to have the hidden type `ConcreteType`. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust // The function item whose generic params we want to merge. fn foo<T>(t: T) -> T { t } // Helper ensuring we can constrain `T` on `F` without explicitly specifying it fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } fn k() -> impl Sized { let x = match true { true => { // `f` is `FnDef(foo, [infer_var])` let f = foo; // Get a value of an opaque type on stable let t = k(); // this returns `FnDef(foo, [k::return])` bind(t, f) } false => foo::<()>, }; todo!() } ``` r? `@compiler-errors`

bors · 2024-04-12T15:58:46Z

☀️ Try build successful - checks-actions
Build commit: f0a46ff (f0a46ffcb8bbd220fe569642e5e175207d8e3fea)

oli-obk · 2024-04-12T20:23:04Z

@craterbot check

craterbot · 2024-04-12T20:23:12Z

👌 Experiment pr-123794 created and queued.
🤖 Automatically detected try build f0a46ff
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rfcbot · 2024-04-12T21:41:15Z

🔔 This is now entering its final comment period, as per the review above. 🔔

craterbot · 2024-04-14T14:51:26Z

🚧 Experiment pr-123794 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-04-16T07:03:28Z

🎉 Experiment pr-123794 is completed!
📊 3 regressed and 1 fixed (437394 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

oli-obk · 2024-04-24T09:05:39Z

@bors r-

oli-obk · 2024-04-24T10:05:59Z

@bors r=lcnr

bors · 2024-04-24T10:06:01Z

📌 Commit 5c55d6a has been approved by lcnr

It is now in the queue for this repository.

More DefineOpaqueTypes::Yes This accepts more code on stable. It is now possible to have match arms return a function item `foo::<ConcreteType>` and a function item `foo::<OpaqueTypeInDefiningScope>` in another, and that will constrain `OpaqueTypeInDefiningScope` to have the hidden type `ConcreteType`. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust // The function item whose generic params we want to merge. fn foo<T>(t: T) -> T { t } // Helper ensuring we can constrain `T` on `F` without explicitly specifying it fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } fn k() -> impl Sized { let x = match true { true => { // `f` is `FnDef(foo, [infer_var])` let f = foo; // Get a value of an opaque type on stable let t = k(); // this returns `FnDef(foo, [k::return])` bind(t, f) } false => foo::<()>, }; todo!() } ``` r? `@compiler-errors` cc rust-lang#116652

Rollup of 5 pull requests Successful merges: - rust-lang#122500 (delegation: Support renaming, and async, const, extern "ABI" and C-variadic functions) - rust-lang#123316 (Test `#[unix_sigpipe = "inherit"]` with both `SIG_DFL` and `SIG_IGN`) - rust-lang#123794 (More DefineOpaqueTypes::Yes) - rust-lang#124282 (windows fill_utf16_buf: explain the expected return value) - rust-lang#124308 (Add diagnostic item for `std::iter::Enumerate`) Failed merges: - rust-lang#124136 (Provide more context and suggestions in borrowck errors involving closures) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 6 pull requests Successful merges: - rust-lang#123316 (Test `#[unix_sigpipe = "inherit"]` with both `SIG_DFL` and `SIG_IGN`) - rust-lang#123794 (More DefineOpaqueTypes::Yes) - rust-lang#123881 (Bump Fuchsia versions) - rust-lang#124281 (fix weak memory bug in TLS on Windows) - rust-lang#124282 (windows fill_utf16_buf: explain the expected return value) - rust-lang#124308 (Add diagnostic item for `std::iter::Enumerate`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123794 - oli-obk:define_opaque_types2, r=lcnr More DefineOpaqueTypes::Yes This accepts more code on stable. It is now possible to have match arms return a function item `foo::<ConcreteType>` and a function item `foo::<OpaqueTypeInDefiningScope>` in another, and that will constrain `OpaqueTypeInDefiningScope` to have the hidden type `ConcreteType`. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust // The function item whose generic params we want to merge. fn foo<T>(t: T) -> T { t } // Helper ensuring we can constrain `T` on `F` without explicitly specifying it fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } fn k() -> impl Sized { let x = match true { true => { // `f` is `FnDef(foo, [infer_var])` let f = foo; // Get a value of an opaque type on stable let t = k(); // this returns `FnDef(foo, [k::return])` bind(t, f) } false => foo::<()>, }; todo!() } ``` r? ``@compiler-errors`` cc rust-lang#116652

…ackh726 Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type r? `@compiler-errors` This accepts more code on stable. It is now possible to have match arms return a function item `foo` and a different function item `bar` in another, and that will constrain OpaqueTypeInDefiningScope to have the hidden type ConcreteType and make the type of the match arms a function pointer that matches the signature. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust fn foo<T>(t: T) -> T { t } fn bar<T>(t: T) -> T { t } fn k() -> impl Sized { fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } let x = match true { true => { let f = foo; bind(k(), f) } false => bar::<()>, }; todo!() } ``` cc rust-lang#116652 This is very similar to rust-lang#123794, and with the same rationale: > this is for consistency with `-Znext-solver`. the new solver does not have the concept of "non-defining use of opaque" right now and we would like to ideally keep it that way. Moving to `DefineOpaqueTypes::Yes` in more cases removes subtlety from the type system. Right now we have to be careful when relating `Opaque` with another type as the behavior changes depending on whether we later use the `Opaque` or its hidden type directly (even though they are equal), if that later use is with `DefineOpaqueTypes::No`*

Rollup merge of rust-lang#124297 - oli-obk:define_opaque_types13, r=jackh726 Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type r? `@compiler-errors` This accepts more code on stable. It is now possible to have match arms return a function item `foo` and a different function item `bar` in another, and that will constrain OpaqueTypeInDefiningScope to have the hidden type ConcreteType and make the type of the match arms a function pointer that matches the signature. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust fn foo<T>(t: T) -> T { t } fn bar<T>(t: T) -> T { t } fn k() -> impl Sized { fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } let x = match true { true => { let f = foo; bind(k(), f) } false => bar::<()>, }; todo!() } ``` cc rust-lang#116652 This is very similar to rust-lang#123794, and with the same rationale: > this is for consistency with `-Znext-solver`. the new solver does not have the concept of "non-defining use of opaque" right now and we would like to ideally keep it that way. Moving to `DefineOpaqueTypes::Yes` in more cases removes subtlety from the type system. Right now we have to be careful when relating `Opaque` with another type as the behavior changes depending on whether we later use the `Opaque` or its hidden type directly (even though they are equal), if that later use is with `DefineOpaqueTypes::No`*

Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type r? `@compiler-errors` This accepts more code on stable. It is now possible to have match arms return a function item `foo` and a different function item `bar` in another, and that will constrain OpaqueTypeInDefiningScope to have the hidden type ConcreteType and make the type of the match arms a function pointer that matches the signature. So the following function will now compile, but on master it errors with a type mismatch on the second match arm ```rust fn foo<T>(t: T) -> T { t } fn bar<T>(t: T) -> T { t } fn k() -> impl Sized { fn bind<T, F: FnOnce(T) -> T>(_: T, f: F) -> F { f } let x = match true { true => { let f = foo; bind(k(), f) } false => bar::<()>, }; todo!() } ``` cc rust-lang/rust#116652 This is very similar to rust-lang/rust#123794, and with the same rationale: > this is for consistency with `-Znext-solver`. the new solver does not have the concept of "non-defining use of opaque" right now and we would like to ideally keep it that way. Moving to `DefineOpaqueTypes::Yes` in more cases removes subtlety from the type system. Right now we have to be careful when relating `Opaque` with another type as the behavior changes depending on whether we later use the `Opaque` or its hidden type directly (even though they are equal), if that later use is with `DefineOpaqueTypes::No`*

rustbot assigned compiler-errors Apr 11, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 11, 2024

Noratrieb reviewed Apr 12, 2024

View reviewed changes

tests/ui/fn/fn_def_coercion.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the define_opaque_types2 branch from be168c7 to 4abbda6 Compare April 12, 2024 10:41

oli-obk added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 12, 2024

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 12, 2024

lcnr reviewed Apr 12, 2024

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 12, 2024

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 12, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 16, 2024

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 24, 2024

This comment has been minimized.

Sign in to view

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2024

oli-obk added 2 commits April 24, 2024 10:05

Add some FnDef LUB coercion tests

ca4a18f

Register hidden types when equating function definitions in coercion

5c55d6a

oli-obk force-pushed the define_opaque_types2 branch from f4521f7 to 5c55d6a Compare April 24, 2024 10:05

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2024

fmease mentioned this pull request Apr 24, 2024

Rollup of 5 pull requests #124329

Closed

fmease mentioned this pull request Apr 24, 2024

Rollup of 6 pull requests #124330

Merged

bors merged commit dc81f6d into rust-lang:master Apr 24, 2024
10 checks passed

rustbot added this to the 1.79.0 milestone Apr 24, 2024

apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024

oli-obk mentioned this pull request May 27, 2024

Allow constraining opaque types during various unsizing casts #125610

Merged

traviscross mentioned this pull request May 28, 2024

Stabilize associated type position impl Trait (ATPIT) #120700

Open

3 tasks

lcnr mentioned this pull request May 30, 2024

Add release notes for 1.79.0 #125470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More DefineOpaqueTypes::Yes #123794

More DefineOpaqueTypes::Yes #123794

oli-obk commented Apr 11, 2024 •

edited

Loading

oli-obk commented Apr 12, 2024

rfcbot commented Apr 12, 2024 •

edited by compiler-errors

Loading

jackh726 commented Apr 12, 2024

lcnr Apr 12, 2024

lcnr commented Apr 12, 2024 •

edited

Loading

jackh726 commented Apr 12, 2024

oli-obk commented Apr 12, 2024

bors commented Apr 12, 2024

bors commented Apr 12, 2024

oli-obk commented Apr 12, 2024

craterbot commented Apr 12, 2024

rfcbot commented Apr 12, 2024

craterbot commented Apr 14, 2024

craterbot commented Apr 16, 2024

This comment has been minimized.

oli-obk commented Apr 24, 2024

oli-obk commented Apr 24, 2024

bors commented Apr 24, 2024

More DefineOpaqueTypes::Yes #123794

More DefineOpaqueTypes::Yes #123794

Conversation

oli-obk commented Apr 11, 2024 • edited Loading

oli-obk commented Apr 12, 2024

rfcbot commented Apr 12, 2024 • edited by compiler-errors Loading

jackh726 commented Apr 12, 2024

lcnr Apr 12, 2024

Choose a reason for hiding this comment

lcnr commented Apr 12, 2024 • edited Loading

jackh726 commented Apr 12, 2024

oli-obk commented Apr 12, 2024

bors commented Apr 12, 2024

bors commented Apr 12, 2024

oli-obk commented Apr 12, 2024

craterbot commented Apr 12, 2024

rfcbot commented Apr 12, 2024

craterbot commented Apr 14, 2024

craterbot commented Apr 16, 2024

This comment has been minimized.

oli-obk commented Apr 24, 2024

oli-obk commented Apr 24, 2024

bors commented Apr 24, 2024

oli-obk commented Apr 11, 2024 •

edited

Loading

rfcbot commented Apr 12, 2024 •

edited by compiler-errors

Loading

lcnr commented Apr 12, 2024 •

edited

Loading