Partial methods implementation should match declaration parameter ref kinds#25893
Partial methods implementation should match declaration parameter ref kinds#25893OmarTawfik merged 2 commits intodotnet:dev15.7.xfrom OmarTawfik:fix-25399-partial-methods-different-ref-kind
Conversation
| } | ||
|
|
||
| [Fact] | ||
| public void PartialMethodsConsiderRefKindDifferences_InWithRef() |
There was a problem hiding this comment.
PartialMethodsConsiderRefKindDifferences_InWithRef [](start = 20, length = 50)
Should probably fold out into the test matrix. #Resolved
There was a problem hiding this comment.
Partial methods cannot have out parameters, and there exists already a test for that.
What is the scenario you're proposing?
In reply to: 178658268 [](ancestors = 178658268)
There was a problem hiding this comment.
Discussed offline. Either confirming CS759 is reported, or lookup the symbol table and confirm they're not merged.
In reply to: 178992135 [](ancestors = 178992135,178658268)
| } | ||
|
|
||
| [Fact] | ||
| public void PartialMethodsWithInParameter_WithBody() |
There was a problem hiding this comment.
PartialMethodsWithInParameter_WithBody [](start = 20, length = 38)
in and ByVal can overload each other. Consider adding a test that overloading like that (and other supported overloading over ref-ness, if any) works properly for partial methods. #Closed
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (iteration 1), modulo suggested tests
|
@dotnet/roslyn-compiler can I get another review? |
1 similar comment
|
@dotnet/roslyn-compiler can I get another review? |
|
|
||
| - Visual Studio 2017 version 15.7: https://github.com/dotnet/roslyn/issues/19792 C# compiler will now reject [IsReadOnly] symbols that should have an [InAttribute] modreq, but don't. | ||
| - Visual Studio 2017 version 15.7: https://github.com/dotnet/roslyn/pull/25131 C# compiler will now check `stackalloc T [count]` expressions to see if T matches constraints of `Span<T>`. | ||
| - Visual Studio 2017 version 15.7: https://github.com/dotnet/roslyn/issues/25399 C# compiler will now produce errors if partial methods parameters have different ref-kinds in implementation vs definition. |
There was a problem hiding this comment.
I assume this was blessed by the Compat Council.
If not, please do (before merging).
jcouv
left a comment
There was a problem hiding this comment.
LGTM modulo compat council approval.
|
approved |
Customer scenario
Customers should not be able to implement partial methods having parameters with a different ref kind than the definition. The compiler should produce diagnostics and prevent that. Currently, the following invalid code passes without errors:
Since this is a breaking change, @dotnet/roslyn-compat-council will be notified and will get approval before merging.
Bugs this fixes
Fixes #25399
Workarounds, if any
None.
Risk
Low risk. This is producing an error in a corner case scenario where code shouldn't have been allowed in first place.
Performance impact
Negligible.
Is this a regression from a previous update?
It is a breaking change.
How was the bug found?
Reported on GitHub by a contributor.
cc @dotnet/roslyn-compiler for review.