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

Explicitly specify type parameter on FromResidual for Option and ControlFlow. #128954

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Aug 11, 2024

Remove type parameter default R = <Self as Try>::Residual from FromResidual Specify default type parameter on FromResidual impls in the stdlib to work around #99940 / #87350 as mentioned in #84277 (comment).

This does not completely fix the issue, but works around it for Option and ControlFlow specifically (Result does not have the issue since it already did not use the default parameter of FromResidual).

(Does this need an ACP or similar?) This probably needs at least an FCP since it changes the API described in the RFC. Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api). This probably doesn't need T-lang input, since it is not changing the API of FromResidual from the RFC? Maybe needs T-libs-api FCP?

@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2024

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 11, 2024
@rust-log-analyzer

This comment has been minimized.

@zachs18 zachs18 force-pushed the fromresidual-no-default branch from 0bd3dd8 to eff3994 Compare August 11, 2024 04:09
@joboet
Copy link
Member

joboet commented Aug 11, 2024

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 11, 2024
@rustbot rustbot assigned m-ou-se and unassigned joboet Aug 11, 2024
@WaffleLapkin
Copy link
Member

From the issue it seems that the default here should work and @lcnr is working on a compiler fix. Do we really need this temporary fix?

@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 12, 2024
@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2024

We expect to fix the underlying issue here in the medium term, so afaict the impact of this change is to prevent some weird errors when implementing FromResidual without specifying the defaulted param? Because removing the default itself doesn't change anything, it just forces people to manually specify the argument for the param (in which case they likely use a manually normalized version)

@zachs18
Copy link
Contributor Author

zachs18 commented Aug 12, 2024

We expect to fix the underlying issue here in the medium term

Okay, if that is expected to happen before try_trait_v2 stabilizes, then this change is probably unnecessary, yeah.

the impact of this change is to prevent some weird errors when implementing FromResidual without specifying the defaulted param

Kind of, but the errors are nonlocal to the impl using defaulted param, as shown in #99940. The errors occur when implementing FromResidual<Local> for Upstream at all for a type where the upstream crate implemented FromResidual for Upstream (e.g. impl<T> FromResidual</* local */ Flip<T>> for ::core::option::Option<T>). Since the errors only occur downstream crates, it's not easy to work around by manually specifying the parameter in the upstream crate.

Because removing the default itself doesn't change anything, it just forces people to manually specify the argument for the param (in which case they likely use a manually normalized version)

It doesn't change anything within a crate, but across crates, it makes FromResidual<Local> for Upstream possible.


Alternately, this PR could be changed to leave the R = <Self as Try>::Residual, but still manually specify the type on all impls in the stdlib, which should resolve specifically not being able to implement FromResidual<Local> for Option/ControlFlow, without changing the API of FromResidual itself. Actually that seems like a much better idea, I'm going to change this PR to do that.

@zachs18

This comment was marked as resolved.

@rustbot rustbot 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 Aug 12, 2024
@zachs18 zachs18 force-pushed the fromresidual-no-default branch from eff3994 to 7e72781 Compare August 12, 2024 16:35
@zachs18 zachs18 changed the title Remove type parameter default from FromResidual Explicitly specify type parameter on FromResidual for Option and ControlFlow. Aug 12, 2024
To work around coherence issue. Also adds regression test.
@zachs18 zachs18 force-pushed the fromresidual-no-default branch from 7e72781 to 1b6df71 Compare August 12, 2024 17:55
@zachs18
Copy link
Contributor Author

zachs18 commented Aug 12, 2024

@rustbot ready

This PR now does not modify FromResidual itself, instead only explicitly specifying the generic for the Option and ControlFlow's FromResidual impls in core, to work around #99940 and allow downstream impl FromResidual<Local> for Option<_>. Also adds a regression test library/core/tests/ops/from_residual.rs to ensure Option, ControlFlow, and Result can have downstream impl FromResidual<Local> impls.

This does not change the API except with respect to the coherence issue. Once the compiler fix happens, the explicit generics should be able to be removed without changing the API.

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

Updating just the impl seems entirely fine to me, especially since any third-party impls are necessarily unstable.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2024

📌 Commit 1b6df71 has been approved by scottmcm

It is now in the queue for this repository.

@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 Aug 14, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 14, 2024
…=scottmcm

Explicitly specify type parameter on FromResidual for Option and ControlFlow.

~~Remove type parameter default `R = <Self as Try>::Residual` from `FromResidual`~~ _Specify default type parameter on `FromResidual` impls in the stdlib_ to work around rust-lang#99940 / rust-lang#87350 ~~as mentioned in rust-lang#84277 (comment).

This does not completely fix the issue, but works around it for `Option` and `ControlFlow` specifically (`Result` does not have the issue since it already did not use the default parameter of `FromResidual`).

~~(Does this need an ACP or similar?)~~ ~~This probably needs at least an FCP since it changes the API described in [the RFC](rust-lang/rfcs#3058). Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api).~~ This probably doesn't need T-lang input, since it is not changing the API of `FromResidual` from the RFC? Maybe needs T-libs-api FCP?
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#128828 (`-Znext-solver` caching)
 - rust-lang#128954 (Explicitly specify type parameter on FromResidual for Option and ControlFlow.)
 - rust-lang#129054 (Subtree update of `rust-analyzer`)
 - rust-lang#129059 (Record the correct target type when coercing fn items/closures to pointers)
 - rust-lang#129071 (Port `run-make/sysroot-crates-are-unstable` to rmake)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#128570 (Stabilize `asm_const`)
 - rust-lang#128828 (`-Znext-solver` caching)
 - rust-lang#128954 (Explicitly specify type parameter on FromResidual for Option and ControlFlow.)
 - rust-lang#129059 (Record the correct target type when coercing fn items/closures to pointers)
 - rust-lang#129071 (Port `run-make/sysroot-crates-are-unstable` to rmake)
 - rust-lang#129088 (Make the rendered html doc for rustc better)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 049b3e5 into rust-lang:master Aug 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
Rollup merge of rust-lang#128954 - zachs18:fromresidual-no-default, r=scottmcm

Explicitly specify type parameter on FromResidual for Option and ControlFlow.

~~Remove type parameter default `R = <Self as Try>::Residual` from `FromResidual`~~ _Specify default type parameter on `FromResidual` impls in the stdlib_ to work around rust-lang#99940 / rust-lang#87350 ~~as mentioned in rust-lang#84277 (comment).

This does not completely fix the issue, but works around it for `Option` and `ControlFlow` specifically (`Result` does not have the issue since it already did not use the default parameter of `FromResidual`).

~~(Does this need an ACP or similar?)~~ ~~This probably needs at least an FCP since it changes the API described in [the RFC](rust-lang/rfcs#3058). Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api).~~ This probably doesn't need T-lang input, since it is not changing the API of `FromResidual` from the RFC? Maybe needs T-libs-api FCP?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API 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