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

Interpretation of ByRefLikeAttribute in .NET Core 2.1 is a breaking change and a standard violation #10446

Closed
gfraiteur opened this issue Jun 4, 2018 · 9 comments

Comments

@gfraiteur
Copy link

@gfraiteur gfraiteur commented Jun 4, 2018

As a matter of facts, it is possible in .NET Core 2.0 to use a ref struct as an argument of a generic parameter if you're generating MSIL, but this same MSIL code no longer runs in .NET Core 2.1. We used this compilation pattern in PostSharp and did all the testing in .NET Core 2.0, just to see it's breaking in .NET Core 2.1.

I understand why you guys have taken this approach as this is a pragmatic way, but I think it is my role as a third-party tool developer to respectfully complain.

Giving a runtime meaning to a custom attribute (ByRefLikeAttribute) is non-standard-compliant (ECMA-335), so the least I would expect is that you document an extension or violation of the standard. An orthodox approach would have been to define a new TypeAttribute and to cope with the problem that the compiler will need a specific version of the runtime.

The concrete impact for this change and as far as PostSharp is concerned is minimal because support for C# 7.2 in PostSharp wasn't made RTW yet.

However, as a policy, breaking standards and relying on "tricks" (like ObsoleteAttribute) to minimize their impact is unhealthy and warrants this complain.

It seems there's a need for a well-thought solution to this problem. Like .NET Standard is the API-level answer to this issue, should it be some CLR Standard? Maybe the compiler could emit a list of non-standard-compliant extensions, marked as MUST UNDERSTAND or OPTIONAL, that all members of the MSIL pipeline (therefore including the compiler, PostSharp and the CLR) would respect.

[assembly: AssemblyRequiresClr( 2012 /* baseline CIL spec */, "!RefStruct" /* required extension */ )]
@sharwell
Copy link
Member

@sharwell sharwell commented Jun 4, 2018

💭 Looks to be introduced by dotnet/coreclr#15745, where the current behavior was desired from the start but wasn't reported in some cases.

@gfraiteur
Copy link
Author

@gfraiteur gfraiteur commented Jun 4, 2018

I want to add that this situation would be a major issue if the breaking change was introduced as a .NET Framework update, as it would break production code.

@jkotas
Copy link
Member

@jkotas jkotas commented Jun 4, 2018

Thank you for reporting this issue. I am sorry for the pain that this is causing to PostSharp.

We have been historically trying hard to make new C# language features work on existing runtimes. It often results in shards like what is described here because of it is not possible to go back in time and fix the existing runtimes to work great with the new features. Another recent example of such shard is the recently introduced unmanaged constraint. It suffers from similar problem - the existing runtimes do not know about it.

We are re-evaluting the strategy going forward. I agree that we need to document this better.

cc @MadsTorgersen @jaredpar

.NET Framework update

The proper runtime support by byref-like types and features that depend on it (e.g. fast span) won't ever show up in .NET Framework update for this reason.

@gfraiteur
Copy link
Author

@gfraiteur gfraiteur commented Jun 4, 2018

No damage/pain for us this time but it would be alarming if this became a trend.

Could we start a discussion about the 'MUST UNDERSTAND or OPTIONAL extension' idea? It does not seem a big implementation work but it could provide a long-term solution to this issue. Of course this itself is a breaking change ;).

@jkotas
Copy link
Member

@jkotas jkotas commented Jun 4, 2018

Going forward, I think we would rather want to make the new features like this work on new runtimes only. It is very simple to explain and reason about.

@gfraiteur
Copy link
Author

@gfraiteur gfraiteur commented Jun 4, 2018

So you would update the schema version in the cor metadata header? It seems what has been designed for.

@stakx
Copy link
Collaborator

@stakx stakx commented Jun 5, 2018

Could we start a discussion about the 'MUST UNDERSTAND or OPTIONAL extension' idea?

As someone who is active in projects that often deal with low-level, runtime & reflection bits (e.g. Moq, Castle DynamicProxy), I would welcome such a discussion.

I'm going off a bit on a tangent here, but the terms that @gfraiteur is using ("must understand" and "optional") happen to coincide with the custom modifiers ("cmods") mechanism that the ECMA 335 has long known. I'm worried about one thing here: Reflection's support for cmods is somewhat spotty, and partially broken. (I can provide evidence if required.) Unlike C++/CLI, C# didn't use cmods for a long time, but it has now started using them for things such as the in parameter modifier. So the limitations of Reflection are now becoming apparent to a larger group of developers, and C#'s use of cmods can pose unsolvable problems for libraries that work with Reflection.

I'd be really happy if the C# language design team and the (Core)CLR developers made sure that Reflection is not left behind, or it'll become increasingly difficult or even impossible to work with.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@mangod9
Copy link
Member

@mangod9 mangod9 commented Sep 17, 2020

Hi @gfraiteur, assume this issue could be closed now, or is there anything actionable to keep it active? Thx.

@gfraiteur
Copy link
Author

@gfraiteur gfraiteur commented Sep 18, 2020

No problem to close.

@gfraiteur gfraiteur closed this Sep 18, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants