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 support for calling local functions with dynamic args #10710

Merged
merged 3 commits into from
Apr 21, 2016

Conversation

agocke
Copy link
Member

@agocke agocke commented Apr 19, 2016

Only exception is params parameters, tracked by #10708.

Fixes #10389

@jaredpar @VSadov @dotnet/roslyn-compiler Please review.

var args = resolution.AnalyzedArguments.Arguments;
var localFunction = validResult.Member;
var parameters = localFunction.Parameters;
var methodResult = validResult.Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pleas replace all vars with types.

@agocke
Copy link
Member Author

agocke commented Apr 21, 2016

@AlekseyTs I tried a slightly different approach to looking for dynamic arguments to a params parameter. Let me know if it seems reasonable to you.

@@ -76,6 +76,33 @@ void VerifyDiagnostics(string source, CSharpCompilationOptions options, params D
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Add [CompilerTrait(CompilerFeature.Dynamic)]

@jaredpar
Copy link
Member

👍

// normal and expanded form i.e., there is exactly one dynamic
// argument to a params parameter
if (OverloadResolution.IsValidParams(localFunction) &&
args.Count == parameters.Length)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2016

Choose a reason for hiding this comment

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

I don't think the second condition should be met. Consider this for example:

    static void Test4(int x = 0, params int[] y)
    {
        dynamic z = x;
        Test4(y: z);
    }

Should probably add a unit-test for similar scenario.

// because there is an ambiguity between an array target
// and the params element target. See
// https://github.com/dotnet/roslyn/issues/10708
Debug.Assert(resolution.OverloadResolutionResult.Succeeded);
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering how you handle "call by name" part when you do not know the mangled name of the method.

I see how :-) It is not supported for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct -- that'll be a completely different implementation, so I didn't want to mix these things together in the same PR.

@VSadov
Copy link
Member

VSadov commented Apr 21, 2016

LGTM
have a test involving optional parameters as Aleksey suggests.
And arglist/vararg as well, if local function can parse that that at all. It is likely to be some kind of error.

@agocke agocke force-pushed the add-dynamic-local-func branch from e0ec4be to 0d13966 Compare April 21, 2016 20:16
@agocke
Copy link
Member Author

agocke commented Apr 21, 2016

@VSadov I think __arglist is out of scope for this PR, but I filed an issue to look into it here: #10765

@AlekseyTs
Copy link
Contributor

Should the test be also marked as LocalFunctions tests?

@AlekseyTs
Copy link
Contributor

LGTM

@agocke agocke merged commit 1548892 into dotnet:future Apr 21, 2016
@agocke agocke deleted the add-dynamic-local-func branch April 21, 2016 21:57
@jaredpar
Copy link
Member

@AlekseyTs the test class is marked as a [CompilerTrait(CompilerFeature.LocalFunctions)]. That gets inheritted by all of the test within it.

@AlekseyTs
Copy link
Contributor

@jaredpar Thanks.

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

Successfully merging this pull request may close these issues.

5 participants