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.

Sign up for GitHub

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

Jump to bottom

Params Collections - Adjust binding in presence of dynamic arguments #71421

Merged
merged 4 commits into from
Jan 8, 2024
Merged

Params Collections - Adjust binding in presence of dynamic arguments #71421

merged 4 commits into from
Jan 8, 2024

Conversation

AlekseyTs
Copy link
Contributor

Implement changes proposed in dotnet/csharplang#7792.

@AlekseyTs AlekseyTs requested review from a team as code owners December 28, 2023 16:32
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 28, 2023
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jan 4, 2024 •
edited 

    // Produce an error. This cannot work correctly right now

I am going to remove the comments in the test scenario since they are no longer accurate. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:4393 in 4e11e24. [](commit_id = 4e11e24, deletion_comment = False)

333fred
333fred approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Generally, this LGTM. We'll need to see what LDM thinks of the single overload static resolution change here though; I personally am a bit concerned by it from a purely backcompat perspective (though I do think it's more consistent behavior overall).

src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs
}

[Fact]
public void DynamicInvocation_OrdinaryMethod_13_DoNotHideByApplicability()
Copy link
Member

@333fred 333fred Jan 3, 2024 •
edited by AlekseyTs

Choose a reason for hiding this comment

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

What does this test have to do with params collections? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does this test have to do with params collections?

The test is not specific to "Params Collections", but the PR also has much broader impact. The implemented design goes beyond "Params Collections".

src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs
""";
var comp = CreateCompilation(src, targetFramework: TargetFramework.StandardAndCSharp, options: TestOptions.ReleaseExe);

comp.VerifyDiagnostics(
Copy link
Member

@333fred 333fred Jan 3, 2024 •
edited by AlekseyTs

Choose a reason for hiding this comment

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

Why not run this one? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not run this one?

It is not my goal to test dynamic dispatch. The warning is what I wanted to see. BTW, I think runtime binder is going to throw for this scenario.

src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs
}

[Fact]
public void DynamicInvocation_Indexer_13_DoNotHideByApplicability()
Copy link
Member

@333fred 333fred Jan 3, 2024 •
edited by AlekseyTs

Choose a reason for hiding this comment

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

Same question here about what this has to do with params collections. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question here about what this has to do with params collections.

It is related to the changes I made in OverloadResolution.cs. The changes have broader impact.

src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs
// M2(in d, d = 3, in d);
Diagnostic(ErrorCode.ERR_InDynamicMethodArg, "d").WithLocation(23, 28)
);
CompileAndVerify(comp, expectedOutput:
Copy link
Member

@333fred 333fred Jan 3, 2024 •
edited by AlekseyTs

Choose a reason for hiding this comment

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

If this is going to work, I think we should update the comments in the source that explicitly say this doesn't work. #Resolved

AlekseyTs
AlekseyTs commented Jan 4, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

ReportBadDynamicArguments(syntax, args, refKindsArray, diagnostics, queryClause);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReportBadDynamicArguments(syntax, args, refKindsArray, diagnostics, queryClause);

Note, I intentionally removed this check, because I believe the spec doesn't require to perform it for statically bound invocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, the helper implements "It is a compile time error if a method invocation is dynamically bound and any of the parameters, including the receiver, has the in modifier." rule from https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/expressions.md#1231-general

@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @dotnet/roslyn-compiler For the second review

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @dotnet/roslyn-compiler For the second review

@RikkiGibson RikkiGibson self-assigned this Jan 5, 2024
@AlekseyTs AlekseyTs enabled auto-merge (squash) January 8, 2024 22:04
@AlekseyTs AlekseyTs merged commit a91ef66 into dotnet:features/ParamsCollections Jan 8, 2024
23 of 27 checks passed
@dotnet-bot dotnet-bot mentioned this pull request Mar 21, 2024
Closed
@333fred 333fred mentioned this pull request Mar 22, 2024
Merged
@jcouv jcouv mentioned this pull request Apr 5, 2024
Closed
333fred added a commit that referenced this pull request Apr 5, 2024
@333fred
Disallow dynamic calls on ref struct receivers (#72674)
7f05bbb 
Fixes #72606. Note that the behavior in the bug has slightly changed from the sharplab build used in the repro due to #71421.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers New Feature - ParamsCollections untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AlekseyTs @333fred @RikkiGibson