Skip to content

regression: Join impl for OsStr insufficiently generic #99683

Closed as not planned
Closed as not planned
regression: Join impl for OsStr insufficiently generic#99683
Labels
T-libs-api[DEPRECATED; DO NOT USE]regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.
Milestone
1.63.0

Description

@Mark-Simulacrum
Mark-Simulacrum
opened on Jul 24, 2022

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).

Activity

  1. Mark-Simulacrum
    added
    T-libs-api[DEPRECATED; DO NOT USE]
    regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
    I-libs-api-nominated[DEPRECATED; DO NOT USE]
    on Jul 24, 2022
  2. Mark-Simulacrum
    added this to the 1.63.0 milestone on Jul 24, 2022
  3. rustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jul 24, 2022

  4. mqudsi commented on Jul 25, 2022

    @mqudsi
    mqudsi
    on Jul 25, 2022
    Contributor

    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?

  5. joshtriplett commented on Jul 27, 2022

    @joshtriplett
    joshtriplett
    on Jul 27, 2022
    Member

    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.

  6. yaahc commented on Jul 27, 2022

    @yaahc
    yaahc
    on Jul 27, 2022
    Member

    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 ↩

  7. rfcbot commented on Jul 27, 2022

    @rfcbot
    rfcbot
    on Jul 27, 2022

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

    Is this breakage acceptable?

  8. mqudsi commented on Jul 27, 2022

    @mqudsi
    mqudsi
    on Jul 27, 2022
    Contributor

    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

  9. est31 commented on Jul 27, 2022

    @est31
    est31
    on Jul 27, 2022
    Member

    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.

  10. yaahc commented on Jul 27, 2022

    @yaahc
    yaahc
    on Jul 27, 2022
    Member

    #26780 (comment)

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

  11. mqudsi commented on Jul 28, 2022

    @mqudsi
    mqudsi
    on Jul 28, 2022
    Contributor

    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?).

  12. yaahc commented on Jul 28, 2022

    @yaahc
    yaahc
    on Jul 28, 2022
    Member

    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.

  13. Mark-Simulacrum
    added
    regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.
    and removed
    regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
    on Aug 7, 2022

  14. Mark-Simulacrum commented on Aug 7, 2022

    @Mark-Simulacrum
    Mark-Simulacrum
    on Aug 7, 2022
    MemberAuthor

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

  15. Mark-Simulacrum
    removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    I-libs-api-nominated[DEPRECATED; DO NOT USE]
    on Aug 9, 2022

  16. m-ou-se commented on Aug 17, 2022

    @m-ou-se
    m-ou-se
    on Aug 17, 2022
    Member

    Closing this as acceptable.

  17. m-ou-se
    closed this as not plannedon Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    T-libs-api[DEPRECATED; DO NOT USE]regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions