-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Test plan followup #44257
Test plan followup #44257
Conversation
@AlekseyTs @dotnet/roslyn-compiler for review. |
default: | ||
return s.ContainingAssembly; | ||
return assemblyIsInReferences(s.ContainingAssembly); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return assemblyIsInReferences(s.ContainingAssembly); [](start = 28, length = 52)
It looks like for a NamedType which isn't a definition we should check all type arguments, including from containing types. Could be a breaking change though. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var funcPtr = (IFunctionPointerTypeSymbol)s; | ||
if (!isContainingAssemblyInReferences(funcPtr.Signature.ReturnType)) | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false [](start = 39, length = 5)
Is this code path covered by tests? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
if (!isContainingAssemblyInReferences(param.Type)) | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false; [](start = 36, length = 13)
Is this code path covered by tests? #Closed
@@ -25,6 +25,7 @@ internal enum TypeCompareKind | |||
IgnoreNullableModifiersForReferenceTypes = 8, | |||
ObliviousNullableModifierMatchesAny = 16, | |||
IgnoreNativeIntegers = 32, | |||
FunctionPointerRefMatchesOutInRefReadonly = 64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionPointerRefMatchesOutInRefReadonly [](start = 8, length = 41)
Consider adding a doc comment, including a brief explanation when is this needed. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it feels strange to have an "ignore" option that is not part of AllIgnoreOptions
. Might want to explain this as well.
In reply to: 426085624 [](ancestors = 426085624)
@@ -313,6 +313,10 @@ internal class MemberSignatureComparer : IEqualityComparer<Symbol> | |||
_considerCallingConvention = considerCallingConvention; | |||
_considerRefKindDifferences = considerRefKindDifferences; | |||
_typeComparison = typeComparison; | |||
if (!considerRefKindDifferences) | |||
{ | |||
_typeComparison |= TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly [](start = 35, length = 57)
Can we assert that this option is never set on entry? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I'm not sure what invariant that would be trying to preserve? Could you elaborate on what type of bug you think that would catch?
In reply to: 426087571 [](ancestors = 426087571)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I'm not sure what invariant that would be trying to preserve? Could you elaborate on what type of bug you think that would catch?
From reviewing the change, I assumed that TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly
is a very special flag. It is meant to be used by this component and it is really not appropriate (suspicious) to use it anywhere else in the code base. If the assumption is correct, the assert would explicitly indicate the intent and could possibly prevent people from misusing the flag.
In reply to: 426889306 [](ancestors = 426889306,426087571)
It feels like this should be relaxed, since we are relaxing RefKind comparison. #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs:400 in 447d87f. [](commit_id = 447d87f, deletion_comment = False) |
It feels like this should be relaxed, since we are relaxing RefKind comparison. #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerParameterSymbol.cs:63 in 447d87f. [](commit_id = 447d87f, deletion_comment = False) |
{ | ||
if ((compareKind & TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly) != 0) | ||
{ | ||
return (refKind1, refKind2) switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(refKind1, refKind2) switch [](start = 23, length = 27)
Why so complicated? It looks like MemberSignatureComparer simply does (refKind1 == RefKind.None) == (refKind2 == RefKind.None)
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this significantly more readable than the version in MemberSignatureComparer
.
In reply to: 426089324 [](ancestors = 426089324)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this significantly more readable than the version in MemberSignatureComparer.
I spent significant amount of time trying to figure out your intent here (time I could have spent elsewhere) and whether all the right combinations are handled and handled properly. Every reader will have to go through the same trouble over and over again. I understand your desire to use switch expressions everywhere you can, but in this case, I believe it makes code less clear and unnecessarily complicated. Please use simplified form of the check, especially that we are already using it elsewhere.
In reply to: 426821892 [](ancestors = 426821892,426089324)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your desire to use switch expressions everywhere you can
To be clear, that is not the motivation.
I spent significant amount of time trying to figure out your intent here (time I could have spent elsewhere) and whether all the right combinations are handled and handled properly.
And I spent probably a similar amount of time creating a truth table in my head for the original check to be sure that I could understand what it was checking. I used this form because I genuinely find it significantly easier to read. I don't find the other form simpler in any fashion.
In reply to: 426825848 [](ancestors = 426825848,426821892,426089324)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change this. However, I want to be clear that I don't just use new features for the fun of it. I use them when it would help me to read the code better, and I find that writing out the truth table (as I can do concisely here with a switch expression) is significantly easier to understand. I'll change this because we have prior art in the form of a comparison, but in future code that does similar things I'm still going to prefer using the form I understand more easily.
In reply to: 426840351 [](ancestors = 426840351,426825848,426821892,426089324)
@@ -102,7 +102,8 @@ internal static TypeSymbol CopyTypeCustomModifiers(TypeSymbol sourceType, TypeSy | |||
|
|||
Debug.Assert(resultType.Equals(sourceType, TypeCompareKind.IgnoreDynamicAndTupleNames | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes)); // Same custom modifiers as source type. | |||
|
|||
Debug.Assert(resultType.Equals(destinationType, TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds)); // Same object/dynamic, nullability and tuple names as destination type. | |||
// Same object/dynamic, nullability and tuple names as destination type. | |||
Debug.Assert(resultType.Equals(destinationType, TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNativeIntegers)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| TypeCompareKind.IgnoreNativeIntegers [](start = 124, length = 39)
This doesn't feel right. Native int types specified in source should be preserved. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like a bug. Please log an issue, thanks.
In reply to: 426823520 [](ancestors = 426823520,426089925)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the flag and skipped the relevant test.
In reply to: 426894790 [](ancestors = 426894790,426836636,426823520,426089925)
}} | ||
unsafe class Derived : Base | ||
{{ | ||
protected override void M1(delegate*<{type2}> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M1 [](start = 28, length = 2)
Consider asserting what signature do we end up with at the end. #Closed
} | ||
unsafe class Derived : Base | ||
{ | ||
protected override void M1(delegate*<(int i, string s)> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M1 [](start = 28, length = 2)
Consider asserting what signature do we end up with at the end. #Closed
} | ||
unsafe class Derived : Base | ||
{ | ||
protected override void M1(delegate*<string> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M1 [](start = 28, length = 2)
Consider asserting what signature do we end up with at the end. #Closed
} | ||
unsafe class Derived : Base | ||
{ | ||
protected override void M1(delegate*<ref string> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M1 [](start = 28, length = 2)
Consider asserting what signature do we end up with at the end. #Closed
} | ||
unsafe class Derived : Base | ||
{ | ||
protected override void M1(ref delegate*<string> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M1 [](start = 28, length = 2)
Consider asserting what signature do we end up with at the end. #Closed
comp.VerifyDiagnostics( | ||
// (12,13): error CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'delegate*<string>' and 'delegate*<string>' | ||
// _ = b ? ptr1 : ptr3; | ||
Diagnostic(ErrorCode.ERR_InvalidQM, "b ? ptr1 : ptr3").WithArguments("delegate*<string>", "delegate*<string>").WithLocation(12, 13), | ||
// (13,13): error CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'delegate*<string>' and 'delegate*<string?>' | ||
// _ = b ? ptr1 : ptr4; | ||
Diagnostic(ErrorCode.ERR_InvalidQM, "b ? ptr1 : ptr4").WithArguments("delegate*<string>", "delegate*<string?>").WithLocation(13, 13), | ||
// (14,24): warning CS8619: Nullability of reference types in value of type 'delegate*<string?>' doesn't match target type 'delegate*<string>'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delegate* [](start = 140, length = 17)
Shouldn't we infer delegate*<string?>
instead? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are ref
s. Unfortunately, our CSharpErrorMessageFormat does not include return ref kinds, so the error message isn't super clear. I could change that to include them, but I'm unsure of the fully ramifications of such a change.
In reply to: 426093508 [](ancestors = 426093508)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, our CSharpErrorMessageFormat does not include return ref kinds, so the error message isn't super clear.
Since this isn't really a member, perhaps the symbol display for function pointers shouldn't be driven by the options for members and should always display ref
kind because this is important aspect of the type. Same for other things we might be dropping
In reply to: 426844096 [](ancestors = 426844096,426093508)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a note to the tracking issue to consider that change, will resolve this thread.
In reply to: 426910541 [](ancestors = 426910541,426844096,426093508)
Are all consumers passing the right Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:463 in 3d33fe4. [](commit_id = 3d33fe4, deletion_comment = False) |
Done with review pass (iteration 7) #Closed |
We could actually remove this API. There are no consumers. In reply to: 629557451 [](ancestors = 629557451) Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:463 in 3d33fe4. [](commit_id = 3d33fe4, deletion_comment = False) |
Added additional documentation for the TypeCompareKind flag, and targetted tests. Added more testing of other scenarios.
@AlekseyTs addressed feedback. In reply to: 629557943 [](ancestors = 629557943) |
/// for types that only differ by the type of ref they have. | ||
/// </summary> | ||
internal static RefKind GetRefKindForHashCode(RefKind refKind) | ||
=> refKind switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refKind switch [](start = 15, length = 14)
I think it would be more robust and clearer to base the hash code on the value (refKind == RefKind.None)
. There would be clear correspondence to the equality evaluation, consistency between implementation of both APIs would be obvious and wouldn't require any additional maintenance when the set of RefKinds is extended. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also an option to not adjust hash code based on ref kinds.
In reply to: 426986569 [](ancestors = 426986569)
Done with review pass (iteration 8) #Closed |
@AlekseyTs address feedback. In reply to: 630534288 [](ancestors = 630534288) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (iteration 9), assuming CI is passing.
@dotnet/roslyn-compiler for a second review. I'm going to hold off on rebasing until reviews are done. |
@dotnet/roslyn-compiler for a second review please. |
Diagnostic(ErrorCode.ERR_CantChangeReturnTypeOnOverride, "M2").WithArguments("Derived.M2()", "Base.M2()", $"delegate*<{refKindString(refKind1)}string, void>").WithLocation(10, 48 + refKind2.Length) | ||
); | ||
|
||
static string refKindString(string orig) => orig == " " ? "" : orig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refKindString [](start = 26, length = 13)
Could this local function be removed if the RefKind.None
cases were "" rather than " "? #Resolved
var comp = CreateCompilationWithFunctionPointers(@" | ||
unsafe class Base | ||
{ | ||
protected virtual void M1(delegate*<(int, string)> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{}} [](start = 60, length = 4)
Is the inner {}
needed? Same question for tests below. #Resolved
|
||
var b = comp2.GetMember("B").GetPublicSymbol(); | ||
|
||
Assert.Throws<ArgumentException>(() => ((Compilation)comp2).IsSymbolAccessibleWithin(ptr1, b)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsSymbolAccessibleWithin [](start = 72, length = 24)
Why does this throw ArgumentException
rather than returning false
? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagnostic(ErrorCode.ERR_InvalidQM, "b ? ptr5 : ptr8").WithArguments("delegate*<string>", "delegate*<string?>").WithLocation(22, 13), | ||
// (23,24): warning CS8619: Nullability of reference types in value of type 'delegate*<string?>' doesn't match target type 'delegate*<string>'. | ||
// _ = b ? ptr7 : ptr8; | ||
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr8").WithArguments("delegate*<string?>", "delegate*<string>").WithLocation(23, 24) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
); [](start = 12, length = 2)
Are we testing mismatch of parameter types in addition to return types? #Resolved
public void NormalizeTaskTypes_FunctionPointers() | ||
{ | ||
|
||
string source = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line. #Resolved
public void Override_SingleDimensionArraySizesInMetadata() | ||
{ | ||
|
||
var il = @" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line. #Resolved
Assert.Equal(ptr1Ref.GetHashCode(), ptr1RefReadonly.GetHashCode()); | ||
Assert.Equal(ptr2Ref.GetHashCode(), ptr2In.GetHashCode()); | ||
Assert.Equal(ptr2Ref.GetHashCode(), ptr2Out.GetHashCode()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 8, length = 1)
Are we testing symbolEqualityComparer.GetHashCode()
? #Resolved
|
||
comp.VerifyDiagnostics( | ||
// (16,29): warning CS8610: Nullability of reference types in type of parameter 'ptr' doesn't match overridden member. | ||
// protected override void M1(delegate*<string> ptr) {{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{}} [](start = 73, length = 4)
Minor: Comments are stale, here and below. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
Debug integration test failure is #43967. |
Added support for the remaining compiler-side required test plan work. @AlekseyTs @dotnet/roslyn-compiler for review. I recommend reviewing this PR commit-by-commit for ease of review.
Relates to #43321