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

regression: Join impl for OsStr insufficiently generic #99683

Closed
Mark-Simulacrum opened this issue Jul 24, 2022 · 11 comments
Closed

regression: Join impl for OsStr insufficiently generic #99683

Mark-Simulacrum opened this issue Jul 24, 2022 · 11 comments
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

In https://crater-reports.s3.amazonaws.com/beta-1.63-2/beta-2022-07-16/reg/belong-0.1.0/log.txt, the code is using a custom Join trait (https://github.com/rossmacarthur/belong/blob/master/src/util.rs#L61) with a different signature from the impl added in #96881 (taking AsRef<OsStr> rather than &OsStr, I think). The std impl is preferred since slice::join is inherent, so we break this code.

This is the only case found in Crater, so probably acceptable breakage, though we might want to consider expanding our impl (not sure if it'll overlap with something).

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jul 24, 2022
@Mark-Simulacrum Mark-Simulacrum added this to the 1.63.0 milestone Jul 24, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 24, 2022
@mqudsi
Copy link
Contributor

mqudsi commented Jul 25, 2022

It would be nice if the slice_concat_ext impl could also be updated to AsRef<OsStr> but as far as the conflicts go, Path implements AsRef<OsStr>, doesn't implement Join, but could probably benefit from this impl (join an array of paths with a / separator?); I think the real question would be String/str which both implement AsRef<OsStr> but we obviously want them using their own Join impls so that might not be possible?

@joshtriplett
Copy link
Member

joshtriplett commented Jul 27, 2022

We discussed this in today's @rust-lang/libs-api meeting, and we'd be in favor of trying to generalize the Join trait impl to accept AsRef<OsStr> for the separator. That should, among other things, allow [os_str, os_str_2].join(" "), which seems useful.

We'd need to make sure that didn't cause widespread inference breakage.

@yaahc
Copy link
Member

yaahc commented Jul 27, 2022

We discussed this in the libs-api team meeting today1 and our preliminary consensus was that this is acceptable breakage, though I'm going to run a poll to confirm this because we did this weeks meeting async so I'm not sure everyone who wanted to participate in that discussion has done so already.

@rfcbot poll libs-api Is this breakage acceptable?

Also, we're interested in trying out the suggestion to expand the impl and see if we run into any overlap or inference breakage issues.

Last but not least, I'm curious if anyone knows why the existing bound was chosen to be S: Borrow instead of S: AsRef.

edit: race condition

Footnotes

  1. https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meetings.202022-07-27/near/291096605

@rfcbot
Copy link

rfcbot commented Jul 27, 2022

Team member @yaahc has asked teams: T-libs-api, for consensus on:

Is this breakage acceptable?

@mqudsi
Copy link
Contributor

mqudsi commented Jul 27, 2022

Last but not least, I'm curious if anyone knows why the existing bound was chosen to be S: Borrow instead of S: AsRef.

cc @est31

@est31
Copy link
Member

est31 commented Jul 27, 2022

Oh this caused some breakage, I'm sorry. I guess it's legal breakage as per policy, and only one broken crate is not that bad. Still I'm not as happy now as I was when my PR got merged :).

Last but not least, I'm curious if anyone knows why the existing bound was chosen to be S: Borrow instead of S: AsRef.

I just copy pasted one of the existing implementations and then modified it, think it was the one for str. If you check the rustdoc for the Join trait, all of the implementations have Borrow. Why the existing implementations use Borrow, I had not influence on them, but according to git log -S:

So I guess the reason why the other impls use Borrow can be found by in #25162. I don't know enough whether AsRef or Borrow is better to have an opinion on either.

@yaahc
Copy link
Member

yaahc commented Jul 27, 2022

#26780 (comment)

Coherence issues, probably worth reinvestigating to see exactly what the issue was.

@mqudsi
Copy link
Contributor

mqudsi commented Jul 28, 2022

Based off of #25162 (comment), the underlying reason is the ambiguity of Vec::<String>::new().concat() - does that return a Vec<Vec<u8>> or String? Using Borrow instead of AsRef caused that to become deterministic (returning the way more probably desired outcome of type String).

For OsStr, Vec::<&OsStr>::concat() isn't/wasn't valid as it is, and still doesn't become valid with the recent PR as OsStr doesn't implement AsRef<[u8]> -- although from what I understand there are existing issues about making that valid, so we shouldn't skip past this point. So there's no code in the wild that currently does Vec::<&OsStr>::concat(), but we have to decide if the benefits of making concat generic over AsRef<OsStr> instead of Borrow<OsStr> outweigh the ergonomic cost of having to be explicit about the desired return type, e.g. let _: OsString = Vec::<&OsStr>::new().concat() vs let _: Vec<Vec<u8>> = Vec::<&OsStr>::new().concat() (which is extremely unlikely to ever be what anyone wants?).

@yaahc
Copy link
Member

yaahc commented Jul 28, 2022

So there's no code in the wild that currently does Vec::<&OsStr>::concat(), but we have to decide if the benefits of making concat generic over AsRef<OsStr> instead of Borrow<OsStr> outweigh the ergonomic cost of having to be explicit about the desired return type, e.g. let _: OsString = Vec::<&OsStr>::new().concat() vs let _: Vec<Vec<u8>> = Vec::<&OsStr>::new().concat() (which is extremely unlikely to ever be what anyone wants?).

I wonder if having something like generic type parameter defaults would let us have our cake and eat it too in this case. I'm not familiar enough with the slice_concat_trait APIs to know if it would help here, but I'd be on the lookout for missing language features that could unblock ideal versions of this API.

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Aug 7, 2022
@Mark-Simulacrum
Copy link
Member Author

Retagging as a stable/stable regression in anticipation that this will slip into 1.63, and likely not be fixed for a good while.

@Mark-Simulacrum Mark-Simulacrum removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Aug 9, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Aug 17, 2022

Closing this as acceptable.

@m-ou-se m-ou-se closed this as not planned Won't fix, can't repro, duplicate, stale Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants