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

Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type #124297

Merged
merged 2 commits into from
May 23, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 23, 2024

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

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 #116652

This is very similar to #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 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 23, 2024
@oli-obk 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 23, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 23, 2024

@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 => {
            (k(),)
        }
        false => ((),),
    };
    todo!()
}

@rfcbot
Copy link

rfcbot commented Apr 23, 2024

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.

@rfcbot 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 23, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 26, 2024
@rfcbot
Copy link

rfcbot commented Apr 26, 2024

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

@compiler-errors compiler-errors added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2024
@camsteffen
Copy link
Contributor

camsteffen commented May 5, 2024

I think this would resolve #101705?

@compiler-errors
Copy link
Member

@camsteffen: I don't believe so. This PR only has to do with cases where we're coercing to/from opaque impl Trait types. That issue has to do with the fact that we don't coerce tuples field-by-field.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 6, 2024
@rfcbot
Copy link

rfcbot commented May 6, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@jackh726
Copy link
Member

jackh726 commented May 6, 2024

r=me if you don't want to wait for @compiler-errors

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 10, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented May 23, 2024

@bors r=jackh726

@bors
Copy link
Contributor

bors commented May 23, 2024

📌 Commit c24148e has been approved by jackh726

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#124297 (Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type)
 - rust-lang#124516 (Allow monomorphization time const eval failures if the cause is a type layout issue)
 - rust-lang#124976 (rustc: Use `tcx.used_crates(())` more)
 - rust-lang#125210 (Cleanup: Fix up some diagnostics)
 - rust-lang#125409 (Rename `FrameworkOnlyWindows` to `RawDylibOnlyWindows`)
 - rust-lang#125416 (Use correct param-env in `MissingCopyImplementations`)
 - rust-lang#125421 (Rewrite `core-no-oom-handling`, `issue-24445` and `issue-38237` `run-make` tests to new `rmake.rs` format)
 - rust-lang#125438 (Remove unneeded string conversion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit abcf400 into rust-lang:master May 23, 2024
12 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
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`*
@traviscross traviscross added the A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants