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

Add helper method to describe features supported by the runtime #20623

Closed
jcouv opened this issue Mar 14, 2017 · 53 comments
Closed

Add helper method to describe features supported by the runtime #20623

jcouv opened this issue Mar 14, 2017 · 53 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Mar 14, 2017

As we ramp the compiler and runtimes together, we want to prevent users from compiling code that we know will fail against a certain runtime.
For instance, we plan for newer runtimes to support default implementations in interface, and the compiler should be able to detect that you're compiling against an older runtime without that support.

Previously, this was done by having the compiler look for APIs that were added coincidentally with the new support. But as we expect to run into this problem more often, it makes sense to have a systematic design for testing feature support.

The main consumer of this API would be the compiler. Whenever it needs to check for runtime support, it would look for the corresponding well-known enum member (for instance, System.Runtime.CompilerServices.RuntimeCapabilities.SupportsDefaultImplementation). If the member exists, then the feature check is successful (the feature is supported). The value for that enum member is ignored.

A number of feature requests or known bugs interfering with compiler or language evolution is documented in dotnet/csharplang#251

Proposed API

(Updated after API review 3/28/2017)
(Updated to use "const" instead of "static readonly" after further discussion 4/13/2017)

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeature
    {
        // Presence of the field indicates runtime support
        public const string FixedGenericAttributes = nameof(FixedGenericAttributes);
        public const string DefaultImplementation = nameof(DefaultImplementation);

        // Runtime check
        public static bool IsSupported(string feature);
    }
}

// Usage

if (RuntimeFeature.IsSupported("SomeNewerFeature")
{
      // Use feature when emitting code at runtime (CodeDOM, RefEmit etc)
}
  • Static detection. The presence of a field alone indicates whether the code compiled against it can rely on the runtime providing the feature. This allows us to model runtime capabilities like any other API, which includes leveraging our existing infrastructure for detecting breaking changes. Also, this allows us to express this in .NET Standard and will version like any other API.

  • Runtime detection. There are some (very limited) scenarios where someone might want to do feature detection at runtime, in particular runtime code generation using RefEmit. We don't expect many developers to call this API. Usage of this API will always involve a literal, as having a field in scope would imply IsSupported must return true. Developers using light-up would discover the strings by looking at the documentation of a particular .NET platform.

Original proposed API

(rejected in favor of the one above)

namespace System.Runtime.CompilerServices
{
    public enum RuntimeFeatures
    {
        FixedGenericAttributes,
        SupportsDefaultImplementation,
        // ... this list would grow over time
     }
}

Maintenance process

Adding new enum members would go through the standard API review process.
The main requirement is that enum names should mean the same thing across the different runtimes (desktop, core, RT, mono).
Also, there needs to be documented agreement between the runtimes and compilers about the meaning of each new member.

CC @gafter, @AlekseyTs, @VSadov, @jaredpar

@jcouv
Copy link
Member Author

jcouv commented Mar 14, 2017

CC @marek-safar to represent mono.

@marek-safar
Copy link
Contributor

This is useful but I am curious where do you think this API will live or whether it'll always be compiled in to every assembly

@jcouv
Copy link
Member Author

jcouv commented Mar 14, 2017

I think this API should live in core libraries (ie. along with the object type).

@jaredpar
Copy link
Member

jaredpar commented Mar 14, 2017

The API needs to live in corlib: the assembly with no external references and containing System.Object.

The enum describes runtime features hence needs to be in corlib as it is tightly coupled with a given runtime. It's the only assembly that can correctly describe runtime capabilities.

@marek-safar
Copy link
Contributor

That's little complication as there is no single corlib anymore (think of netstandard(s)) but I guess runtime does not really need to read this enum type declaration after all.

@jaredpar
Copy link
Member

The runtime doesn't need to read this enum. But the compiler needs to know the capabilities of a runtime to decide what features it can enable. This problem exists precisely because there are so many runtimes out there now 😦

Consider default interface methods as an example. When targeting Desktop 4.6 this feature can't be enabled. C# though has no notion of TFMs, frameworks, etc ... All the compiler sees is references. The only library we can use to reliably determine runtime features is corlib, hence this is where the type needs to be.

@karelz
Copy link
Member

karelz commented Mar 14, 2017

cc @terrajobst

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 15, 2017

As an alternative to an enum I would like to suggest to use a static class with Boolean constant fields instead. So, something like:

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeatures
    {
        public const bool FixedGenericAttributes = true;
        public const bool SupportsDefaultImplementation = false;
        // ... this list would grow over time
     }
}

This way we will be able to rely on the value of a particular field vs. just its presence or absence. It will be much easier to right conditional code based on the fact that a certain feature is supported. For example:

if (RuntimeFeatures.TheFeatureIAmInterestedIn)
{
    // Take advantage of the feature
}
else
{
    // The feature is not supported, perform an appropriate recovery.
}

@jcouv
Copy link
Member Author

jcouv commented Mar 15, 2017

From discussion with Aleksey, we came up with a design that addresses both compile-time and runtime-time scenarios reasonably well.

namespace System.Runtime.CompilerServices
{
    public static class Runtime
    {
        [Supports(
            RuntimeFeatures.FixedGenericAttributes, 
            RuntimeFeatures.SupportsDefaultImplementation)]
        bool Supports(RuntimeFeatures value)
        {
            switch (value) { ... } // this implementation needs to match the declaration in attribute above
        }
    }

    public class SupportsAttribute : Attribute
    {
         public SupportsAttribute(params RuntimeFeatures[] supported) { ... }
    }
}

Checks at compile-time would inspect the Supports attribute, and checks at runtime would call the bool Supports(RuntimeFeatures) method.

@svick
Copy link
Contributor

svick commented Mar 16, 2017

  1. How will this work with .Net Standard? Do I understand it correctly that newer versions of .Net Standard reference assemblies will expand the list of features in [Supports] and by doing that requiring all frameworks that implement that version of .Net Standard to support those runtime features (and not just API surface)?

  2. How do you make a check at runtime for a feature that is not listed in the version of RuntimeFeatures you're targeting at compile time? By specifying the numeric value of the enum value, e.g. Runtime.Supports((RuntimeFeatures)42)?

    In case that's not clear, imagine that .Net 7 adds a new FooFeature = 42; to RuntimeFeatures and at the same time to Supports. I'm targeting .Net 6 at compile time, but my code might actually be running on .Net 7, so I want to test for FooFeature. But I can't reference FooFeature directly, because it doesn't exist in .Net 6 reference assemblies. But I can find out what is the numeric value of FooFeature on .Net 7 and use that.

  3. If my understanding of 2. above is correct, is it expected that a value will be added to RuntimeFeatures, without being added to Supports in the same version too? In other words, can I actually expect Runtime.Supports(RuntimeFeatures.SomeValueICanReferenceAtCompileTime) to ever return false?

@jaredpar
Copy link
Member

@svick

How will this work with .Net Standard?

It doesn't. Net Standard describes the set of APIs that are available to a program. This particular enum is describing the set of runtime features available to compilers.

Concrete example: there is a PR out to add support for generic attributes. This isn't about API surface but rather enabling a particular scenario at the CLR level. Not all runtimes support this feature. Hence in order to know whether or not to allow the feature the compiler needs to understand if the runtime is going to support it.

How do you make a check at runtime for a feature that is not listed in the version of RuntimeFeatures you're targeting at compile time?

The compiler is mostly just cracking the metadata and looking for the existence of features that is is aware of.

@svick
Copy link
Contributor

svick commented Mar 16, 2017

@jaredpar

How will this work with .Net Standard?

It doesn't. Net Standard describes the set of APIs that are available to a program. This particular enum is describing the set of runtime features available to compilers.

Concrete example: there is a PR out to add support for generic attributes. This isn't about API surface but rather enabling a particular scenario at the CLR level. Not all runtimes support this feature. Hence in order to know whether or not to allow the feature the compiler needs to understand if the runtime is going to support it.

So if I write a library that targets .Net Standard x.y, I will never be able to use generic attributes? Because the compiler doesn't know what runtime it will run on, only that I target that specific version of .Net Standard.

The compiler is mostly just cracking the metadata and looking for the existence of features that it is aware of.

Yes, but the latest proposal by @jcouv implied that there are run-time scenarios as well. I was specifically asking about those.

@jaredpar
Copy link
Member

So if I write a library that targets .Net Standard x.y, I will never be able to use generic attributes?

This will be true for Net Standard 2.0. It's possible in the future Net Standard will begin requiring that runtimes support generic attributes and it will be included in the definition of corlib.

@terrajobst
Copy link
Member

terrajobst commented Mar 18, 2017

I like the idea to use APIs to describe the runtime capabilities so that we can model which features a runtime implementing a specific .NET Standard version has to support. This allows compilers to rely on that.

The existence of an attribute application is too subtle for my taste: it's easily missed by regular diffing tools and not covered by our existing API compliance tools. We could of course fix this, but I'd rather see an actual API delta (missing type or missing field).

Also, what's the value of dynamic checking? What code would use that and how would that work?

Tagging @migueldeicaza, as he seemed to have concerns as well.

@jkotas
Copy link
Member

jkotas commented Mar 18, 2017

Also, what's the value of dynamic checking? What code would use that and how would that work?

If you want to do a light up in a library.

There are really two distinct cases to think about:

  • New compiler features running into bugs in existing runtimes. It is the case of the generic attributes mentioned above. You can use them fine, as long as you do dot use the one reflection API that fails on them. I do not think it makes sense to block language features just because of you can run into a runtime bug with it. It will get ugly fast if it gets pushed to extreme.
  • The new major features like the default methods on interfaces. It is what this issue should be really about.

@benaadams
Copy link
Member

A related area is Vectors where IsHardwareAccelerated was the go to. However there are newer apis like Narrow and Widen which will only be hardware acclerated on newer (as yet un-merged) jits; so would also need detection.

@VSadov
Copy link
Member

VSadov commented Mar 19, 2017

I could see how feature employing dynamic codegen (like dlr) could benefit from dynamic checks for jit bugs and such.

As a general light-up - not sure. The alternative code paths would still need to be compiled even if not used.

@migueldeicaza
Copy link
Contributor

I can not pinpoint exactly the problem, but the proposal generally leaves a bad taste in my mouth.

We seem to be mixing up two issues here: how should the compiler choose what code it generates, and later in the discussion we added a discussion on how this might be potentially useful at runtime.

The former problem seems to be, how should the compiler decide how to generate code. It seems that we have made a leap from the requirement to the implementation specific detail that is to embed into mscorlib a fingerprint that describes the capabilities, and then rely on this as the mechanism to control the code generation.

This is new territory in the compiler world. Other languages rely on compile-time options to drive the compilation process, and it is a process that is well understood with well-understood side effects. I do not have a complete grasp of what the side effects of these choices will be right now.

The second capability, the runtime capability to probe for features is a very useful one, but one that I do not believe should rely on enumeration values. I would like to have something like:

public static object ProbeCapability (string capabiltyName)

We would have to standardize the capability names, but it would allow code that is compiled with an older toolchain to lightup with new toolchains. The string return can be used either for a quick bool on/off, or can be used to return a capability-specific payload:

if (System.ProbeCapbility ("can-eat-cookies") != null)){ ... }

if (System.ProbeCapability ("supported-formats") as string p && p.Contains ("json/utf8")) {...}

@terrajobst
Copy link
Member

terrajobst commented Mar 28, 2017

We've discussed this today between us and the compiler guys. Here is what we left with:

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeature
    {
        // Presence of the field indicates runtime support
        public static readonly string FixedGenericAttributes = nameof(FixedGenericAttributes);
        public static readonly string DefaultImplementation = nameof(DefaultImplementation);

        // Runtime check
        public static bool IsSupported(string feature);
    }
}

// Usage

if (RuntimeFeature.IsSupported("SomeNewerFeature")
{
      // Use feature when emitting code at runtime (CodeDOM, RefEmit etc)
}
  • Static detection. The presence of a field alone indicates whether the code compiled against it can rely on the runtime providing the feature. This allows us to model runtime capabilities like any other API, which includes leveraging our existing infrastructure for detecting breaking changes. Also, this allows us to express this in .NET Standard and will version like any other API.

  • Runtime detection. There are some (very limited) scenarios where someone might want to do feature detection at runtime, in particular runtime code generation using RefEmit. We don't expect many developers to call this API. Usage of this API will always involve a literal, as having a field in scope would imply IsSupported must return true. Developers using light-up would discover the strings by looking at the documentation of a particular .NET platform.

@jcouv, could update your proposal above to reflect this?

@jcouv
Copy link
Member Author

jcouv commented Mar 28, 2017

@terrajobst Updated proposal description to match the agreement you documented. Thanks

@marek-safar
Copy link
Contributor

marek-safar commented Mar 28, 2017

Do I understand it correctly that

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeature
    {
        public static bool IsSupported(string);
    }
}

will exist in netstandard.dll API and when executed the runtime specific mscorlib will handle the true/false logic.

@karelz
Copy link
Member

karelz commented Mar 29, 2017

The API approval is for .NET Core only. We will update .NET Standard in future version when all/most platforms support the API. All platforms that can support it should do it.

@terrajobst
Copy link
Member

@marek-safar

Since .NET Standard 2.0 can only model existing APIs it will not have RuntimeFeature in 2.0. I expect this type and some of the runtime features to become available with .NET Standard 2.1.

@VSadov
Copy link
Member

VSadov commented Apr 7, 2017

Just thought of a runtime capability, that would be convenient to check for - ability to unload types/assemblies.

I have seen many examples where code goes to great lengths trying to not retain runtime type objects - do not put types as keys (unless the type is a known primitive) in Dictionaries/Caches, do not cache delegates .... - That is with the assumption that retaining type objects will prevent assembly unloading.

However, if I am not mistaken, only a few, if any, runtimes can actually unload types/assemblies.

It would be helpful for the dynamic scenarios, to know if unloading is not possible and thus no need to worry about, for example, using type objects as keys in caches.

This is basically an ask for a capability string like

public static readonly string CollectibleAssemblies = nameof(CollectibleAssemblies);

Any better name is more than welcome.

@VSadov
Copy link
Member

VSadov commented Apr 7, 2017

Added the "CollectibleAssemblies" capability string as a separate API proposal
https://github.com/dotnet/corefx/issues/18087

@MichalStrehovsky
Copy link
Member

Is there a particular reason why the switches are static readonly instead of const? If their only purpose in life is for the compiler to probe for them, we could at least save us from emitting a class constructor that initializes them...

@jcouv
Copy link
Member Author

jcouv commented Apr 8, 2017

@MichalStrehovsky Yes, static readonly is on purpose. If I understood correctly, there is a problem with const's possibly getting inlined, which defeats the purpose of runtime detection.

@jkotas
Copy link
Member

jkotas commented Apr 8, 2017

This does not make sense to me. Could you please explain in detail how const would defeat the runtime detection?

@marek-safar
Copy link
Contributor

It could defeat the runtime detection in const case if a value of the "feature" field/const is implementation detail (e.g each runtime can use its own value, for example shorter string for faster compares) but it's not clear from the proposal if that's the intention as it mixes readonly field and literal value together.

@jkotas
Copy link
Member

jkotas commented Apr 10, 2017

The static fields cannot be used in combination with the runtime detection. If the runtime does not support the feature, it won't have the matching static field.

@OmarTawfik OmarTawfik changed the title Add enum to describe features supported by the runtime Add helper method to describe features supported by the runtime Apr 10, 2017
@terrajobst
Copy link
Member

Is there a particular reason why the switches are static readonly instead of const? If their only purpose in life is for the compiler to probe for them, we could at least save us from emitting a class constructor that initializes them...

I originally proposed them to be const. @gafter preferred them to be static readonly as this would allow the C#/VB compilers to include a field ref in the member ref table in case the emitted code would start depending on these features. This would allow forcing a runtime failure in case an assembly is being loaded (by accident) on an older runtime that doesn't actually support the feature.

I don't believe this scenario is very sensible to defend against; in practice (for instance in the .NET Framework) most binaries compiled against any 4.x framework will run on an older runtime and only fail for cases where an API is being used that didn't exist before. Thus, apps that depend on new behavior might fail in less deterministic ways. This is usually not a problem because apps have app.config (generated by VS) that declares it needs the later version of the framework. Our shim will use that information to present a prompt asking the customer to install a later version of the .NET Framework.

For .NET Core, a similar thing holds as the app's runtime.json indicates which version of .NET Core the app needs to run on. If you try running it on an older framework it will deterministically fail at startup. And for self-contained apps, the app developer virtually has to go out of their way to screw it.

So yes, we should change the fields to become const. This also expresses the intent that their values can never change.

@jcouv
Copy link
Member Author

jcouv commented Apr 13, 2017

Updated the description (up top) to use const.

@OmarTawfik
Copy link
Contributor

@jcouv @terrajobst I'll send another PR to update the comment with the proposed usage.

@jcouv
Copy link
Member Author

jcouv commented Apr 24, 2017

@OmarTawfik I suspect this can be closed (coreCLR and mscorlib/desktop are done, and type-forwards were added).

@OmarTawfik
Copy link
Contributor

@jcouv I'll wait for nightly build to do one more verification.

@OmarTawfik
Copy link
Contributor

@jcouv @dotnet/roslyn-compiler This is now done and merged.

@karelz
Copy link
Member

karelz commented Apr 25, 2017

@OmarTawfik can you please link PR # or commit #?

@OmarTawfik
Copy link
Contributor

@karelz
Adding type to runtime: dotnet/coreclr#10779
Updating comments to describe intended behaviour: dotnet/coreclr#10945
Forwarders in corefx: dotnet/corefx#18637

@karelz
Copy link
Member

karelz commented Apr 25, 2017

Thanks!

@OmarTawfik
Copy link
Contributor

@marek-safar any information needed on my side for mono?

@marek-safar
Copy link
Contributor

@OmarTawfik What .net version will support this API?

@agocke
Copy link
Member

agocke commented Apr 27, 2017

cc @tmat, who I think was also interested in this feature.

@tmat
Copy link
Member

tmat commented Apr 27, 2017

Thanks @agocke. I think we should add value that indicates the runtime supports Portable PDBs, so that we can detect that when compiling at runtime and opt into more efficient PDBs.
//cc @noahfalk

@OmarTawfik
Copy link
Contributor

@marek-safar core 2.0 and desktop 4.7.1.

@tmat
Copy link
Member

tmat commented May 15, 2017

We are finishing implementation of Portable PDBs for Desktop FX 4.7.1

I'd like to add

System.Runtime.CompilerServices.RuntimeFeature.PortablePdb

to Desktop FX in the same check-in. Any objections?

@noahfalk FYI

@jaredpar
Copy link
Member

@tmat can you expand on what support went into desktop for portable PDB? Hard to understand where this may get used based on the limited information in the comment.

@tmat
Copy link
Member

tmat commented May 15, 2017

@jaredpar Currently there is no support for Portable PDBs in Desktop FX. We are finishing support in stack traces for portable and embedded PDBs for 4.7.1. Features that generate code at runtime (such as scripting) would benefit from being able to detect whether the runtime supports Portable PDBs or not, as they could emit Portable PDBs instead of Windows PDBs. Emitting Portable PDBs has perf benefits.

@jcouv
Copy link
Member Author

jcouv commented May 15, 2017

@tmat, please start an new CoreFX API proposal (linked to this one). Every new feature the runtime declares will go through new API discussion/review.

@karelz
Copy link
Member

karelz commented May 15, 2017

Do you need it in .NET Core 2.0? We should be forking into rel/2.0.0 branch today.

@tmat
Copy link
Member

tmat commented May 15, 2017

@karelz It would be nice to have it there too. Not strictly necessary since there are other ways to detect Core CLR, but it would certainly simplify things. I can prepare PR in a few minutes.

@karelz
Copy link
Member

karelz commented May 15, 2017

@tmat you need API review first. Then we can talk about PR.
If it is not must-have in .NET Core 2.0, then let's not do it there - we are way past feature complete. We do not want feature creep. Let's rather focus on 2.0 feedback.

@tmat
Copy link
Member

tmat commented May 15, 2017

OK.

@tmat
Copy link
Member

tmat commented May 15, 2017

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests