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

Support non-trailing named arguments with dynamic. #2069

Open
gafter opened this issue Dec 12, 2018 · 0 comments
Open

Support non-trailing named arguments with dynamic. #2069

gafter opened this issue Dec 12, 2018 · 0 comments

Comments

@gafter
Copy link
Member

gafter commented Dec 12, 2018

@JonHanna commented on Thu Jan 18 2018

Currently with the likes of:

dynamic s = "abcabc";
int i = s.IndexOf(value: 'a', 2);

We get CS8324.

Since support for non-trailing named arguments has recently been merged into corefx, ideally roslyn could make use of it, rather than raising that error.

In itself this is simple, but identifying whether it will be supported or not is another matter.


@jcouv commented on Sat Jan 20 2018

Tagging @VSadov to advise.
Can or should the compiler try to detect runtime capability to decide whether or not to let such code compile?
We've introduced such an API (RuntimeFeature) not long ago. Corefx could add RuntimeFeature.NonTrailingNamedArgumentInDynamicInvocation, then the compiler could gate its behavior on that.


@VSadov commented on Sat Jan 20 2018

Yes. We should add some handshake and condutionally enable the scenario.

It would be something similar to the “RuntimeFeature” but in dynamic binder. Perhaps simpler - we probably only need the static aspect of it.


@VSadov commented on Sat Jan 20 2018

Actually we may add support for ‘in’ argument kind in the binder and use that as a trigger that both ‘in’ arguments and nontrailing names are supported


@VSadov commented on Sat Jan 20 2018

Although it feels a bit like hack.

More formal “feature X supported” marker like in RuntimeFeature is probably better.


@JonHanna commented on Sat Jan 20 2018

We should probably allow for in anyway, since it's feasible (for some reason I had convinced myself it wasn't, probably jumping to false conclusions by analogy to ref on arguments that are themselves dynamic rather than accompanying dynamic). It's not going to be completely backwards compatible due to the way it affects overloading (unless we insist that in must be included at the call site rather than being optional, but that seems a difference from static code that will lead to confusion), but the incompatibilities should be rare in practice.

It does seem a bit of a hack to use the relevant enum field to indicate support for a feature that could exist without it (indeed, which does exist without it right now), but it's a hack that needs no further API work, and seems it should be dependable.


@VSadov commented on Sat Jan 20 2018

in at the call site is meaningful for the purpose of overload resolution, so it must be passed to the binder. For that CSharpArgumentInfoFlags needs a new entry. And compiler can use the presence of the entry as a key that the feature is supported.

As for the nontrailing names - there is no special API to check. And I feel there will be more features like this. The solution could be something like RuntimeFeature , but in Microsoft.CSharp namespace.

namespace Microsoft.CSharp.CompilerServices
{
    public static class RuntimeFeature
    {
        // Presence of the field indicates runtime support
        public const string NontrailingNamedArguments = nameof(NontrailingNamedArguments);

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

// Usage

if (RuntimeFeature.IsSupported("SomeNewerFeature")
{
      // Use feature when emitting code at runtime (CodeDOM, RefEmit etc)
}

@jcouv commented on Wed Jan 24 2018

Sounds good. I will bring this up with LDM first (probably can get it on the agenda for next week).

@JonHanna Would you be able to help drive such API on corefx side (once LDM gives thumbs up)?

Some thoughts ahead of the corefx/API discussion:
I'm not completely sure we need a RuntimeFeature API in the Microsoft.CSharp namespace, as opposed to simply extending the existing API.
If package dependencies and versioning work, I think we can ensure that when Microsoft.CompilerServices.RuntimeFeature.IsSupported("NonTrailingNamedArguments") returns true, then the dynamic binder necessarily supports the feature. If you use an older binder, it depends on an older core library where this API returns false.


@JonHanna commented on Wed Jan 24 2018

Would you be able to help drive such API on corefx side

Sure.

If package dependencies and versioning work, I think we can ensure…

Is it possible to use a more recent version of just Microsoft.CSharp? (I pretty much just use what nuget gives me across the board myself, so don't know from experience) and hence for someone to have support for that version while CompilerServices wouldn't think they should?


@JonHanna commented on Wed Jan 24 2018

If this can be done from the existing API. I wonder if we should start to namespace the string identifiers a bit, so something like "Microsoft.CSharp:NonTrailingNamedArguments". The list of features is something that is only going to get larger, and eventually get to the point where most people will not know what most features are and be lost as to where to look to find out, and which they can quickly dismiss as not of interest to them.


@jcouv commented on Wed Jan 24 2018

There is a way to ensure that the new Microsoft.CSharp binaries depend on new core library: just reference the new member Microsoft.CompilerServices.RuntimeFeature.NonTrailingNamedArguments in code.

I don't have a strong opinion on the identifier for this feature, but keep in mind that the pattern for RuntimeFeature is to add corresponding properties, but property names can't have punctuation.


@gafter commented on Tue Dec 11 2018

Moving to csharplang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant