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

[release/9.0] Remove string.Trim{,Start,End}(ReadOnlySpan<char>) ref-API #108777

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented Oct 11, 2024

Manual backport of #108537 to release/9.0

Customer Impact

  • Customer reported
  • Found internally

Customers who have existing extension methods, string.Trim(string) (or TrimEnd, or TrimStart) find that their extension methods no longer get called after upgrading to .NET 9. These extension methods almost always want to turn "prefixinfixsuffix".TrimEnd("suffix") into "prefixinfix", but our new method public [instance] string [string::]TrimEnd(ReadOnlySpan<char> value) has higher binding order over their extension method, and it produces the result "prefixin", as it is instead treating the "suffix" as a set of characters to remove from the end of the string (equivalent to "prefixinfixsuffix".TrimEnd('s', 'u', 'f', 'f', 'i', 'x'), rather than a specific sequence of characters.

Regression

  • Yes
  • No

Customers who had applicable extension methods experience these new overloads as a regression from previous versions.

Testing

As the tests for these methods no longer compile, they are also removed.

Risk

This change is breaking. Any callers who specifically call Trim, TrimStart, or TrimEnd with a ReadOnlySpan<char> will have to change their code. Two such sites were identified in dotnet/wpf and have already been mitigated.

Callers which were calling the new methods via params syntax will not experience a runtime break with this change, as it removes the ref but not the implementation (yet). When those callers recompile they will switch (back) to the Trim(params char[]) overload with no action required on their part.

As this change only removes things, the risk that the change does not solve the problem is low. The risk that it requires unanticipated followup in dotnet/runtime is also low. While other dotnet/* repositories have been checked for breaks, some may have been missed, and they will break automatic ingestion and have to be fixed as they surface.

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Oct 11, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@bartonjs bartonjs added area-System.Runtime Servicing-consider Issue for next servicing release review breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed new-api-needs-documentation needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 11, 2024
Copy link
Contributor

dotnet-policy-service bot commented Oct 11, 2024

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

<Target>M:System.String.TrimStart(System.ReadOnlySpan{System.Char})</Target>
</Suppression>
</Suppressions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we haven't already, I assume our intent is to follow-up for net10.0 with a change to remove these and delete the source impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current intent is both main and ~9.0.1, just letting prebuilt things get through a cycle of the method "not being there" without the MissingMethodException.

@bartonjs
Copy link
Member Author

Breaking change documentation seeded at dotnet/docs#43032.

@jeffhandley jeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Oct 11, 2024
@carlossanlop
Copy link
Member

/ba-g failure is preexisting

@carlossanlop carlossanlop merged commit b030c4d into dotnet:release/9.0 Oct 11, 2024
148 of 163 checks passed
@bartonjs bartonjs deleted the remove_trim_span9 branch October 11, 2024 22:22
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants