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/8.0-preview7] Workaround a C++/CLI bug involving DIMs #89264

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 20, 2023

Backport of #89253 to release/8.0-preview7

/cc @jeffhandley @tannergooding

Customer Impact

The dotnet/wpf repo is currently blocked trying to build with Preview 7, and backporting this workaround to Preview 7 will unblock the builds. Using C++/CLI cannot currently compile applications referencing .NET 8 Preview 7, and this workaround unblocks the builds.

Testing

Ensured C++/CLI compilation succeeds with this change in place

Risk

There is currently a C++/CLI bug involving implementing an instance DIM on a derived interface. This causes C++/CLI to error when using any type that implements such an interface, which in the case of System.Numerics.INumberBase<TSelf> includes all the primitive types (such as Int32 or Single).

There were a few possible changes that would temporarily resolve this issue...

  1. This change, where we stop implementing the interface but keep exposing the general functionality on the interface until we can revert.
  2. We could have updated IUtf8SpanFormattable to have a DIM where it always throws PlatformNotSupportedException, but this could lead to unexpected behavior for consumers of the API
  3. We could have updated IUtf8SpanFormattable to have a DIM where it always returns false, but this risks users naively doing the wrong thing and trying to grow the buffer and call the API repeatedly as that's how the other TryFormat APIs work
  4. We could have updated IUtf8SpanFormattable to inherit from ISpanFormattable and have a DIM where it defers to that implementation

Of these options, 1 or 4 are the least risky.

4 would end up changing the shipping API surface and would not require a reversion later. It has the downside in that it requires everyone implementing IUtf8SpanFormattable to also support ISpanFormattable (that is UTF-16). This puts more restriction on the API surface overall. That being said, it is the better option if we believe that the C++/CLI fix will not make the .NET 8 release as avoids us introducing a diamond problem in the future as the DIM is provided in the same release that the relevant interface and API are defined.

1 (this change) will require us to revert this PR once the C++/CLI fix goes in. Not reverting this workaround will result in INumberBase<TSelf> not being able to implement IUtf8SpanFormattable without risking the introduction of a diamond problem, since other types/interfaces could implement IUtf8SpanFormattable with a DIM before the .NET 9 release.

@dotnet-issue-labeler
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.

@ghost
Copy link

ghost commented Jul 20, 2023

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #89253 to release/8.0-preview7

/cc @jeffhandley @tannergooding

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@jeffhandley jeffhandley added this to the 8.0.0 milestone Jul 20, 2023
@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Jul 20, 2023
@carlossanlop carlossanlop merged commit ebd2346 into release/8.0-preview7 Jul 20, 2023
168 of 173 checks passed
@carlossanlop carlossanlop deleted the backport/pr-89253-to-release/8.0-preview7 branch July 20, 2023 20:06
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants