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 #18280

Open
gfraiteur opened this Issue Jun 4, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@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

This comment has been minimized.

Member

sharwell commented Jun 4, 2018

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

@gfraiteur

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment