Skip to content

Commit

Permalink
Fix sorting of extensions from more specific to less (#73276)
Browse files Browse the repository at this point in the history 
Also disallows resolution of extension members in using directives
Also adds a constraint check when substituting compatible extensions
  • Loading branch information
@jcouv
jcouv committed May 9, 2024
1 parent 6fcb813 commit 8d2e7af
Show file tree
Hide file tree
Showing 37 changed files with 1,099 additions and 355 deletions.
9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/BinderFlags.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Microsoft.CodeAnalysis.CSharp
/// A specific location for binding.
/// </summary>
[Flags]
internal enum BinderFlags : uint
internal enum BinderFlags : ulong
{
None, // No specific location
SuppressConstraintChecks = 1 << 0,
Expand Down Expand Up @@ -116,7 +116,12 @@ internal enum BinderFlags : uint
/// Indicates the binder is used during collection expression conversion
/// to verify applicable methods are available.
/// </summary>
CollectionExpressionConversionValidation = 1u << 31,
CollectionExpressionConversionValidation = 1ul << 31,

/// <summary>
/// PROTOTYPE(static) we currently disallow resolving extensions in using directives
/// </summary>
InUsingDirective = 1ul << 32,

// Groups

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11000,7 +11000,7 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
var type = receiver.Type;
Debug.Assert(type is not null);
// Note: extension methods are not allowed to extend an extension type
if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (type.GetExtendedTypeNoUseSiteDiagnostics(null) is { } extendedType)
{
type = extendedType;
}
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Flags.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,13 @@ internal Binder WithCheckedOrUncheckedRegion(bool @checked)
? this
: new Binder(this, (this.Flags & ~removed) | added);
}

/// <summary>
/// PROTOTYPE(static) we currently disallow resolving extensions in using directives
/// </summary>
internal bool InUsingDirective
{
get { return this.Flags.Includes(BinderFlags.InUsingDirective); }
}
}
}
112 changes: 49 additions & 63 deletions src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,20 +188,12 @@ protected void LookupMembersInternal(LookupResult result, NamespaceOrTypeSymbol

var compatibleExtensions = ArrayBuilder<NamedTypeSymbol>.GetInstance();

if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (type.GetExtendedTypeNoUseSiteDiagnostics(basesBeingResolved) is { } extendedType)
{
type = extendedType;
}

GetCompatibleExtensions(this, type, compatibleExtensions, originalBinder, basesBeingResolved);

// Sort extensions from more specific to less specific
var tempUseSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(useSiteInfo);
compatibleExtensions.Sort((x, y) => isMoreSpecificExtension(x, y) ? -1 : 1); // Note: captures tempUseSiteInfo
if (tempUseSiteInfo.AccumulatesDiagnostics)
{
useSiteInfo.MergeAndClear(ref tempUseSiteInfo);
}
GetCompatibleExtensions(this, type, compatibleExtensions, originalBinder, basesBeingResolved, ref useSiteInfo);

// PROTOTYPE test use-site diagnostics
var tempResult = LookupResult.GetInstance();
Expand All @@ -218,31 +210,12 @@ protected void LookupMembersInternal(LookupResult result, NamespaceOrTypeSymbol

tempResult.Free();
compatibleExtensions.Free();
return;

bool isMoreSpecificExtension(NamedTypeSymbol extension, NamedTypeSymbol other)
{
if (extension.ExtendedTypeNoUseSiteDiagnostics is not { } extendedType
|| other.ExtendedTypeNoUseSiteDiagnostics is not { } otherExtendedType)
{
// We wouldn't have gathered these members if we didn't have an extended type for them.
throw ExceptionUtilities.Unreachable();
}

if (extendedType.Equals(otherExtendedType, TypeCompareKind.AllIgnoreOptions))
{
return false;
}

// PROTOTYPE(static) revise if we allow extension lookup on type parameters
Debug.Assert(!extendedType.IsTypeParameter());
Debug.Assert(!otherExtendedType.IsTypeParameter());
return DerivesOrImplements(baseTypeOrImplementedInterface: otherExtendedType, derivedType: extendedType, null, this.Compilation, ref tempUseSiteInfo);
}
}

// Note: we return the compatible extensions in order:
// for any given pair, if one is more specific it will precede the other in the list
private static void GetCompatibleExtensions(Binder binder, TypeSymbol type, ArrayBuilder<NamedTypeSymbol> compatibleExtensions,
Binder originalBinder, ConsList<TypeSymbol>? basesBeingResolved)
Binder originalBinder, ConsList<TypeSymbol>? basesBeingResolved, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
if (type.IsErrorType())
{
Expand All @@ -252,47 +225,59 @@ bool isMoreSpecificExtension(NamedTypeSymbol extension, NamedTypeSymbol other)
var extensions = ArrayBuilder<NamedTypeSymbol>.GetInstance();
binder.GetImplicitExtensionTypes(extensions, originalBinder);

foreach (var extension in extensions)
#if !DEBUG
// In DEBUG mode, we prefer to exercise the code below even in the absence of extensions
if (extensions.Count == 0)
{
return;
}
#endif

PooledHashSet<NamedTypeSymbol>? visited = null;
var baseType = type;
do
{
if (basesBeingResolved?.Contains(extension) == true)
addCompatibleExtensions(binder, extensions, baseType, compatibleExtensions, basesBeingResolved);
baseType = baseType.GetNextBaseTypeNoUseSiteDiagnostics(basesBeingResolved, originalBinder.Compilation, ref visited);
}
while (baseType is not null);

if (type is NamedTypeSymbol namedType)
{
foreach (var implementedInterface in GetBaseInterfaces(namedType, basesBeingResolved, ref useSiteInfo))
{
continue;
addCompatibleExtensions(binder, extensions, implementedInterface, compatibleExtensions, basesBeingResolved);
}

addSubstitutedIfCompatible(extension, type, compatibleExtensions);
}

visited?.Free();
extensions.Free();
return;

static void addSubstitutedIfCompatible(NamedTypeSymbol extension, TypeSymbol type, ArrayBuilder<NamedTypeSymbol> compatibleExtensions)
static void addCompatibleExtensions(Binder binder, ArrayBuilder<NamedTypeSymbol> extensions, TypeSymbol type,
ArrayBuilder<NamedTypeSymbol> compatibleExtensions, ConsList<TypeSymbol>? basesBeingResolved)
{
Debug.Assert(!type.IsExtension);
if (extension.ExtendedTypeNoUseSiteDiagnostics is null)
foreach (var extension in extensions)
{
return;
}

var baseType = type;
do
{
if (TypeUnification.CanImplicitlyExtend(extension, baseType, out AbstractTypeParameterMap? map))
Debug.Assert(extension.IsDefinition);
if (extension.GetExtendedTypeNoUseSiteDiagnostics(basesBeingResolved) is null)
{
var substitutedExtension = map is not null ? (NamedTypeSymbol)map.SubstituteType(extension).Type : extension;
compatibleExtensions.Add(substitutedExtension);
break;
continue;
}

baseType = baseType.BaseTypeNoUseSiteDiagnostics;
}
while (baseType is not null);

foreach (var implementedInterface in type.AllInterfacesNoUseSiteDiagnostics)
{
if (TypeUnification.CanImplicitlyExtend(extension, implementedInterface, out AbstractTypeParameterMap? map))
if (TypeUnification.CanImplicitlyExtend(extension, type, out AbstractTypeParameterMap? map))
{
var substitutedExtension = map is not null ? (NamedTypeSymbol)map.SubstituteType(extension).Type : extension;
compatibleExtensions.Add(substitutedExtension);

// PROTOTYPE(static) we should warn for nullability issues
var constraintsOk = substitutedExtension.CheckConstraints(
new ConstraintsHelper.CheckConstraintsArgs(binder.Compilation, binder.Conversions, includeNullability: false, Location.None, BindingDiagnosticBag.Discarded));

if (constraintsOk)
{
compatibleExtensions.Add(substitutedExtension);
}
}
}
}
Expand All @@ -316,7 +301,7 @@ protected void LookupMembersInType(LookupResult result, TypeSymbol type, string
var extension = (NamedTypeSymbol)type;
this.LookupMembersInExtension(result, extension, name, arity, basesBeingResolved, options, originalBinder, diagnose, ref useSiteInfo);

if (extension.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (extension.GetExtendedTypeNoUseSiteDiagnostics(basesBeingResolved) is { } extendedType)
{
// any viable non-methods [non-indexers] found here will hide viable methods [indexers] (with the same name) in any further base classes
// short circuit looking up in extended type if we already have a viable result and we won't be adding on more
Expand Down Expand Up @@ -1543,7 +1528,7 @@ private void MergeExtensionLookupResultsHidingLessSpecific(LookupResult resultHi
{
var sym = hiddenSymbols[i];

if (sym.ContainingType.ExtendedTypeNoUseSiteDiagnostics is not { } symExtendedType)
if (sym.ContainingType.GetExtendedTypeNoUseSiteDiagnostics(basesBeingResolved) is not { } symExtendedType)
{
// We wouldn't have gathered these members if we didn't have an extended type for them.
throw ExceptionUtilities.Unreachable();
Expand All @@ -1554,7 +1539,7 @@ private void MergeExtensionLookupResultsHidingLessSpecific(LookupResult resultHi
{
var hidingSym = hidingSymbols[j];

if (hidingSym.ContainingType.ExtendedTypeNoUseSiteDiagnostics is not { } hidingSymExtendedType)
if (hidingSym.ContainingType.GetExtendedTypeNoUseSiteDiagnostics(basesBeingResolved) is not { } hidingSymExtendedType)
{
// We wouldn't have gathered these members if we didn't have an extended type for them.
throw ExceptionUtilities.Unreachable();
Expand Down Expand Up @@ -2207,7 +2192,7 @@ void addMemberLookupSymbolsInfoInExtension(LookupSymbolsInfo result, TypeSymbol
{
AddMemberLookupSymbolsInfoWithoutInheritance(result, type, options, originalBinder, accessThroughType: type);

if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (type.GetExtendedTypeNoUseSiteDiagnostics(null) is { } extendedType)
{
AddMemberLookupSymbolsInfoInType(result, extendedType, options, originalBinder, accessThroughType: type);
}
Expand Down Expand Up @@ -2383,15 +2368,16 @@ private void AddMemberLookupSymbolsInfoInTypeParameter(LookupSymbolsInfo result,
internal void AddImplicitExtensionMemberLookupSymbolsInfoForType(LookupSymbolsInfo result, TypeSymbol type, LookupOptions options, Binder originalBinder)
{
var accessThroughType = type;
if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (type.GetExtendedTypeNoUseSiteDiagnostics(null) is { } extendedType)
{
type = extendedType;
}

var compatibleExtensions = ArrayBuilder<NamedTypeSymbol>.GetInstance();
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
foreach (var scope in new ExtensionScopes(this))
{
GetCompatibleExtensions(scope.Binder, type, compatibleExtensions, originalBinder, basesBeingResolved: null);
GetCompatibleExtensions(scope.Binder, type, compatibleExtensions, originalBinder, basesBeingResolved: null, ref discardedUseSiteInfo);

foreach (NamedTypeSymbol extension in compatibleExtensions)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,8 @@ private NamedTypeSymbol BindPredefinedTypeSymbol(PredefinedTypeSyntax node, Bind
this.LookupSymbolsSimpleName(result, qualifierOpt, identifierValueText, 0, basesBeingResolved, options, diagnose: true, useSiteInfo: ref useSiteInfo);
diagnostics.Add(node, useSiteInfo);

if (result.Kind != LookupResultKind.Viable && qualifierOpt is TypeSymbol typeSymbol)
// PROTOTYPE(static) we currently disallow resolving extensions in using directives
if (result.Kind != LookupResultKind.Viable && qualifierOpt is TypeSymbol typeSymbol && !InUsingDirective)
{
Debug.Assert(result.Kind != LookupResultKind.Ambiguous);
this.LookupExtensionTypeMembersIfNeeded(result, typeSymbol, identifierValueText, arity: 0, basesBeingResolved, options, ref useSiteInfo);
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1484,7 +1484,7 @@ private void RemoveLessSpecificExtensionMembers<TMember>(ArrayBuilder<MemberReso

if (result.Result.IsValid || result.HasUseSiteDiagnosticToReport)
{
if (result.Member.ContainingType.ExtendedTypeNoUseSiteDiagnostics is not { } extendedType)
if (result.Member.ContainingType.GetExtendedTypeNoUseSiteDiagnostics(null) is not { } extendedType)
{
continue;
}
Expand Down Expand Up @@ -1548,7 +1548,7 @@ static bool isLessDerived(TypeSymbol type, NamedTypeSymbol currentType, ref Comp
}
else if (currentType.IsExtension)
{
if (currentType.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
if (currentType.GetExtendedTypeNoUseSiteDiagnostics(null) is { } extendedType)
{
if (extendedType.Equals(type, TypeCompareKind.ConsiderEverything))
{
Expand Down Expand Up @@ -1607,7 +1607,7 @@ private bool IsLessSpecificThanAnyExtensionMember<TMember>(int index, TypeSymbol
continue;
}

if (result.Member.ContainingType.ExtendedTypeNoUseSiteDiagnostics is not { } currentExtendedType)
if (result.Member.ContainingType.GetExtendedTypeNoUseSiteDiagnostics(null) is not { } currentExtendedType)
{
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ private void CompileSynthesizedExtensionMarker(SourceExtensionTypeSymbol sourceE
if (!_globalHasErrors)
{
var extensionMarker = new SynthesizedExtensionMarker(sourceExtension,
sourceExtension.ExtendedTypeNoUseSiteDiagnostics,
sourceExtension.GetExtendedTypeNoUseSiteDiagnostics(null),
_diagnostics);

#if DEBUG
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/AliasSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ private NamespaceSymbol ResolveExternAliasTarget(BindingDiagnosticBag diagnostic
}

var syntax = usingDirective.NamespaceOrType;
var flags = BinderFlags.SuppressConstraintChecks | BinderFlags.SuppressObsoleteChecks;
var flags = BinderFlags.SuppressConstraintChecks | BinderFlags.SuppressObsoleteChecks | BinderFlags.InUsingDirective;
if (usingDirective.UnsafeKeyword != default)
{
this.CheckUnsafeModifier(DeclarationModifiers.Unsafe, usingDirective.UnsafeKeyword.GetLocation(), diagnostics);
Expand Down
2 changes: 1 addition & 1 deletion ...table/Symbols/AnonymousTypes/PublicSymbols/AnonymousManager.TypeOrDelegatePublicSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ internal sealed override ImmutableArray<NamedTypeSymbol> GetDeclaredInterfaces(C
internal sealed override bool IsExtension => false;
internal sealed override bool IsExplicitExtension => false;

internal sealed override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal sealed override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal sealed override TypeSymbol? GetDeclaredExtensionUnderlyingType()
=> throw ExceptionUtilities.Unreachable();
Expand Down
2 changes: 1 addition & 1 deletion ...e/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.TypeOrDelegateTemplateSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ internal sealed override AttributeUsageInfo GetAttributeUsageInfo()
internal sealed override bool IsExtension => false;
internal sealed override bool IsExplicitExtension => false;

internal sealed override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal sealed override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal sealed override TypeSymbol? GetDeclaredExtensionUnderlyingType()
=> throw ExceptionUtilities.Unreachable();
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/ArrayTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ protected sealed override ITypeSymbol CreateITypeSymbol(CodeAnalysis.NullableAnn
internal override bool IsExtension => false;
internal override bool IsExplicitExtension => false;

internal override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal sealed override IEnumerable<(MethodSymbol Body, MethodSymbol Implemented)> SynthesizedInterfaceMethodImpls()
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/DynamicTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ protected sealed override ITypeSymbol CreateITypeSymbol(CodeAnalysis.NullableAnn
internal override bool IsExtension => false;
internal override bool IsExplicitExtension => false;

internal override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal override IEnumerable<(MethodSymbol Body, MethodSymbol Implemented)> SynthesizedInterfaceMethodImpls()
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/ErrorTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ protected sealed override ITypeSymbol CreateITypeSymbol(CodeAnalysis.NullableAnn
internal sealed override bool IsExtension => false;
internal sealed override bool IsExplicitExtension => false;

internal sealed override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal sealed override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal sealed override TypeSymbol? GetDeclaredExtensionUnderlyingType()
=> throw ExceptionUtilities.Unreachable();
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ internal static bool IsCallingConventionModifier(NamedTypeSymbol modifierType)
internal sealed override bool IsExtension => false;
internal sealed override bool IsExplicitExtension => false;

internal sealed override TypeSymbol? ExtendedTypeNoUseSiteDiagnostics => null;
internal sealed override TypeSymbol? GetExtendedTypeNoUseSiteDiagnostics(ConsList<TypeSymbol>? basesBeingResolved) => null;

internal override IEnumerable<(MethodSymbol Body, MethodSymbol Implemented)> SynthesizedInterfaceMethodImpls()
{
Expand Down
Loading

0 comments on commit 8d2e7af

Please sign in to comment.