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

Workaround a C++/CLI bug involving DIMs #89253

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Jul 20, 2023

As per the title 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 ghost assigned tannergooding Jul 20, 2023
@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

null

Author: tannergooding
Assignees: -
Labels:

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

Milestone: -

@tannergooding tannergooding marked this pull request as ready for review July 20, 2023 16:30
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

This temporary workaround looks good to me, with the understanding that we'll revert it once the C++/CLI team applies their fix and we take that update into dotnet/wpf.

@jeffhandley jeffhandley merged commit 812492e into dotnet:main Jul 20, 2023
168 checks passed
@jeffhandley
Copy link
Member

/backport to release/8.0-preview7

@github-actions
Copy link
Contributor

Started backporting to release/8.0-preview7: https://github.com/dotnet/runtime/actions/runs/5614063183

@tannergooding tannergooding deleted the fix-wpf branch July 20, 2023 18:25
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants