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

Hide ToString specializations behind a private trait #89253

Closed
wants to merge 1 commit into from

Conversation

SkiFire13
Copy link
Contributor

Normally we use private traits for specializing impls but ToString's were public and the original PR #32586 doesn't seem to have a reason for this. This PR moves those impls to a hidden perma-unstable SpecToString trait (I would have made it private if proc_macro didn't have to specialize it too) and delegates to it, leaving only the impl<T: fmt::Display + ?Sized> ToStringSpec for T as public.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

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

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Sep 27, 2021

but isn't proc_macro specializing ToString an example why the default fn should be kept public 🤔

@SkiFire13
Copy link
Contributor Author

proc_macro will still use the specialized version, just not publicly

@kennytm
Copy link
Member

kennytm commented Sep 27, 2021

actually #32586 (comment) questioned the rationale of making ToString publicly specializable, the response was

Very good question. We talked about this some in a recent libs meeting -- ultimately, we're going to want a conventions RFC here, I think. But it's hard to get there without some experimentation first.

what i interpret is that there's no preference in wanting either way. afaik no conventions RFC exists yet. has any experimentation been done?

 

IMO creating a private-but-actually-public SpecToString trait to hide away the specialization possibility does not provide much benefits over keeping ToString specializable, as long as #31844 min_specialization is still unstable.

@SkiFire13
Copy link
Contributor Author

what i interpret is that there's no preference in wanting either way. afaik no conventions RFC exists yet

While not technically being an RFC, the std-dev-guide specifies that specialization should use private traits https://std-dev-guide.rust-lang.org/code-considerations/using-unstable-lang/specialization.html

IMO creating a private-but-actually-public SpecToString trait to hide away the specialization possibility does not provide much benefits over keeping ToString specializable, as long as #31844 min_specialization is still unstable.

I agree with that, however at least the change makes it consistent with how specialization is handled in the rest of the stdlib.

@the8472
Copy link
Member

the8472 commented Sep 27, 2021

IIRC the reason for hiding specializations had something to do with the specialized methods being nameable through fully qualified function calls, e.g. <SpecializedImpl as Trait>::specialized_function. The part that I don't recall is what the negative impact of that was. 😅

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@bors
Copy link
Contributor

bors commented Jan 16, 2022

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

@SkiFire13 SkiFire13 closed this Jan 16, 2022
@the8472 the8472 added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 16, 2022
@SkiFire13 SkiFire13 deleted the hide-tostring-spec branch March 15, 2022 09:01
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants