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

Test plan followup #44257

Merged
merged 11 commits into from
May 20, 2020
Merged

Test plan followup #44257

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6b6689f
Add public API access check support.
333fred May 6, 2020
cae7f81
Function pointer symbols cannot be referenced by name.
333fred May 7, 2020
8c9f326
A PointerType's sort order is never usable today.
333fred May 7, 2020
5d5dcc5
Return true for IsTypeOrTypeAlias for function pointers.
333fred May 8, 2020
3f94e67
Support normalizing task-like types in function pointer types.
333fred May 8, 2020
447d87f
Support for overloading and overriding signatures with function point…
333fred May 13, 2020
3d33fe4
Add nullable tests
333fred May 14, 2020
3f827dd
Respond to PR feedback.
333fred May 18, 2020
ed27295
Adjust correspondence in the GetRefKindForHashCode function to match …
333fred May 19, 2020
232b02e
Adjust tests per feedback.
333fred May 20, 2020
b8418fb
Fixed formatting.
333fred May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2966,26 +2966,28 @@ internal bool HasImplicitPointerConversion(TypeSymbol? source, TypeSymbol? desti
return false;
}

if (!hasConversion(sourceParam.RefKind, destinationSig.Parameters[i].Type, sourceSig.Parameters[i].Type, ref useSiteDiagnostics))
if (!hasConversion(sourceParam.RefKind, destinationSig.Parameters[i].TypeWithAnnotations, sourceSig.Parameters[i].TypeWithAnnotations, ref useSiteDiagnostics))
{
return false;
}
}

return sourceSig.RefKind == destinationSig.RefKind
&& hasConversion(sourceSig.RefKind, sourceSig.ReturnType, destinationSig.ReturnType, ref useSiteDiagnostics);
&& hasConversion(sourceSig.RefKind, sourceSig.ReturnTypeWithAnnotations, destinationSig.ReturnTypeWithAnnotations, ref useSiteDiagnostics);

bool hasConversion(RefKind refKind, TypeSymbol sourceType, TypeSymbol destinationType, ref HashSet<DiagnosticInfo>? useSiteDiagnostics)
bool hasConversion(RefKind refKind, TypeWithAnnotations sourceType, TypeWithAnnotations destinationType, ref HashSet<DiagnosticInfo>? useSiteDiagnostics)
{
switch (refKind)
{
case RefKind.None:
return HasIdentityOrImplicitReferenceConversion(sourceType, destinationType, ref useSiteDiagnostics)
|| HasImplicitPointerToVoidConversion(sourceType, destinationType)
|| HasImplicitPointerConversion(sourceType, destinationType, ref useSiteDiagnostics);
return (!IncludeNullability || HasTopLevelNullabilityImplicitConversion(sourceType, destinationType))
&& (HasIdentityOrImplicitReferenceConversion(sourceType.Type, destinationType.Type, ref useSiteDiagnostics)
|| HasImplicitPointerToVoidConversion(sourceType.Type, destinationType.Type)
|| HasImplicitPointerConversion(sourceType.Type, destinationType.Type, ref useSiteDiagnostics));

default:
return HasIdentityConversion(sourceType, destinationType);
return (!IncludeNullability || HasTopLevelNullabilityIdentityConversion(sourceType, destinationType))
&& HasIdentityConversion(sourceType.Type, destinationType.Type);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ internal bool Equals(FunctionPointerMethodSymbol other, TypeCompareKind compareK

private bool EqualsNoParameters(FunctionPointerMethodSymbol other, TypeCompareKind compareKind, IReadOnlyDictionary<TypeParameterSymbol, bool>? isValueTypeOverride)
=> CallingConvention == other.CallingConvention
&& RefKind == other.RefKind
&& FunctionPointerTypeSymbol.RefKindEquals(compareKind, RefKind, other.RefKind)
&& ((compareKind & TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds) != 0
|| RefCustomModifiers.SequenceEqual(other.RefCustomModifiers))
&& ReturnTypeWithAnnotations.Equals(other.ReturnTypeWithAnnotations, compareKind, isValueTypeOverride);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ internal bool Equals(FunctionPointerParameterSymbol other, TypeCompareKind compa
&& _containingSymbol.Equals(other._containingSymbol, compareKind, isValueTypeOverride);

internal bool MethodEqualityChecks(FunctionPointerParameterSymbol other, TypeCompareKind compareKind, IReadOnlyDictionary<TypeParameterSymbol, bool>? isValueTypeOverride)
=> RefKind == other.RefKind
=> FunctionPointerTypeSymbol.RefKindEquals(compareKind, RefKind, other.RefKind)
&& ((compareKind & TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds) != 0
|| RefCustomModifiers.SequenceEqual(other.RefCustomModifiers))
&& TypeWithAnnotations.Equals(other.TypeWithAnnotations, compareKind, isValueTypeOverride);
Expand Down
17 changes: 17 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,5 +157,22 @@ internal override TypeSymbol SetNullabilityForReferenceTypes(Func<TypeWithAnnota
return this;
}

internal static bool RefKindEquals(TypeCompareKind compareKind, RefKind refKind1, RefKind refKind2)
{
if ((compareKind & TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly) != 0)
{
return (refKind1, refKind2) switch
Copy link
Contributor

@AlekseyTs AlekseyTs May 15, 2020 •
edited
Loading

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

Copy link
Member Author

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)

Copy link
Contributor

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)

Copy link
Member Author

@333fred 333fred May 18, 2020 •
edited
Loading

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)

Copy link
Member Author

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)

{
(RefKind.None, RefKind.None) => true,
(RefKind.None, _) => false,
(_, RefKind.None) => false,
_ => true
};
}
else
{
return refKind1 == refKind2;
}
}
}
}
6 changes: 5 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ internal class MemberSignatureComparer : IEqualityComparer<Symbol>
considerTypeConstraints: false,
considerCallingConvention: false,
considerRefKindDifferences: false,
typeComparison: TypeCompareKind.AllIgnoreOptions); //shouldn't actually matter for source members
typeComparison: TypeCompareKind.AllIgnoreOptions);

/// <summary>
/// This instance is used to determine if a partial method implementation matches the definition.
Expand Down Expand Up @@ -313,6 +313,10 @@ private MemberSignatureComparer(
_considerCallingConvention = considerCallingConvention;
_considerRefKindDifferences = considerRefKindDifferences;
_typeComparison = typeComparison;
if (!considerRefKindDifferences)
{
_typeComparison |= TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly;
Copy link
Contributor

@AlekseyTs AlekseyTs May 15, 2020 •
edited
Loading

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

Copy link
Member Author

@333fred 333fred May 18, 2020 •
edited
Loading

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)

Copy link
Contributor

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)

}
}

#region IEqualityComparer<Symbol> Members
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/CustomModifierUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

@AlekseyTs AlekseyTs May 15, 2020 •
edited
Loading

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@cston can you comment? Regardless I don't want to make a change around native int encoding in this PR, so I'm either going to skip the tests that hit this assert or keep this as is.

In reply to: 426089925 [](ancestors = 426089925)

Copy link
Member

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #44358.

In reply to: 426836636 [](ancestors = 426836636,426823520,426089925)

Copy link
Member Author

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)


return resultType;
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Symbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ public bool CanBeReferencedByName

case SymbolKind.ArrayType:
case SymbolKind.PointerType:
case SymbolKind.FunctionPointer:
case SymbolKind.Assembly:
case SymbolKind.DynamicType:
case SymbolKind.NetModule:
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/SymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ public static bool IsTypeOrTypeAlias(this Symbol symbol)
case SymbolKind.ErrorType:
case SymbolKind.NamedType:
case SymbolKind.PointerType:
case SymbolKind.FunctionPointer:
case SymbolKind.TypeParameter:
return true;
case SymbolKind.Alias:
Expand Down
48 changes: 48 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,13 @@ private static bool NormalizeTaskTypesInType(CSharpCompilation compilation, ref
type = pointerType;
return changed;
}
case SymbolKind.FunctionPointer:
{
var functionPointerType = (FunctionPointerTypeSymbol)type;
var changed = NormalizeTaskTypesInFunctionPointer(compilation, ref functionPointerType);
type = functionPointerType;
return changed;
}
}
return false;
}
Expand Down Expand Up @@ -1722,6 +1729,47 @@ private static bool NormalizeTaskTypesInPointer(CSharpCompilation compilation, r
return true;
}

private static bool NormalizeTaskTypesInFunctionPointer(CSharpCompilation compilation, ref FunctionPointerTypeSymbol funcPtrType)
{
var returnType = funcPtrType.Signature.ReturnTypeWithAnnotations;
var madeChanges = NormalizeTaskTypesInType(compilation, ref returnType);

var paramTypes = ImmutableArray<TypeWithAnnotations>.Empty;

if (funcPtrType.Signature.ParameterCount > 0)
{
var paramsBuilder = ArrayBuilder<TypeWithAnnotations>.GetInstance(funcPtrType.Signature.ParameterCount);
bool madeParamChanges = false;
foreach (var param in funcPtrType.Signature.Parameters)
{
var paramType = param.TypeWithAnnotations;
madeParamChanges |= NormalizeTaskTypesInType(compilation, ref paramType);
paramsBuilder.Add(paramType);
}

if (madeParamChanges)
{
madeChanges = true;
paramTypes = paramsBuilder.ToImmutableAndFree();
}
else
{
paramTypes = funcPtrType.Signature.ParameterTypesWithAnnotations;
paramsBuilder.Free();
}
}

if (madeChanges)
{
funcPtrType = funcPtrType.SubstituteTypeSymbol(returnType, paramTypes, refCustomModifiers: default, paramRefCustomModifiers: default);
return true;
}
else
{
return false;
}
}

internal static Cci.TypeReferenceWithAttributes GetTypeRefWithAttributes(
this TypeWithAnnotations type,
Emit.PEModuleBuilder moduleBuilder,
Expand Down
Loading