Skip to content

Extensions: Adjust accessibility and file-type checks for extension members #77657

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

Merged
merged 1 commit into from
Mar 19, 2025
+454 −57
Merged

Extensions: Adjust accessibility and file-type checks for extension members #77657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/AccessCheck.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,9 @@ private static bool IsMemberAccessible(
return true;
}

// For the purpose of accessibility checks, extension members are considered to be declared within the enclosing static type
return IsNonPublicMemberAccessible(
containingType,
containingType.IsExtension && containingType.ContainingType is { } extensionEnclosingType ? extensionEnclosingType : containingType,
Copy link
Member

@jcouv jcouv Mar 18, 2025

Choose a reason for hiding this comment

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

containingType.IsExtension && containingType.ContainingType is { } extensionEnclosingType

nit: consider using the same pattern you used elsewhere in this PR (containingType is { IsExtension: true, ContainingType: { } extensionEnclosingType } ? extensionEnclosingType : containingType) or even introducing a helper method.

declaredAccessibility,
within,
throughTypeOpt,
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ bool Cci.ITypeDefinition.IsSealed
{
Debug.Assert((object)method != null);

if ((alwaysIncludeConstructors && method.MethodKind == MethodKind.Constructor) || method.GetCciAdapter().ShouldInclude(context))
if ((alwaysIncludeConstructors && method.MethodKind == MethodKind.Constructor) || method is SynthesizedExtensionMarker || method.GetCciAdapter().ShouldInclude(context))
{
yield return method.GetCciAdapter();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Extensions/SynthesizedExtensionMarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal SynthesizedExtensionMarker(SourceMemberContainerTypeSymbol extensionTyp
return;
}

private static DeclarationModifiers GetDeclarationModifiers() => DeclarationModifiers.Public | DeclarationModifiers.Static;
private static DeclarationModifiers GetDeclarationModifiers() => DeclarationModifiers.Private | DeclarationModifiers.Static;
Copy link
Member

@jcouv jcouv Mar 18, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

I don't have any objection to this change. Please update the spec accordingly:

The extension marker method encodes the receiver parameter.

It is public and static, and is called <Extension>$.
It has the attributes, refness, type and name from the receiver parameter on the extension declaration.
If the receiver parameter doesn't specify a name, then the parameter name is empty.
``` #Pending


internal override bool HasSpecialName => true; // PROTOTYPE: reconcile with spec

Expand Down
8 changes: 6 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ internal sealed override ParameterSymbol? ExtensionParameter
var methodSymbol = getMarkerMethodSymbol(@this, uncommon);

// PROTOTYPE: do we want to tighten the flags check further? (require that type be sealed?)
if (methodSymbol.DeclaredAccessibility != Accessibility.Public ||
if (methodSymbol.DeclaredAccessibility != Accessibility.Private ||
methodSymbol.IsGenericMethod ||
!methodSymbol.IsStatic ||
!methodSymbol.ReturnsVoid ||
Expand Down Expand Up @@ -2238,11 +2238,15 @@ private PooledDictionary<MethodDefinitionHandle, PEMethodSymbol> CreateMethods(A
// for ordinary embeddable struct types we import private members so that we can report appropriate errors if the structure is used
var isOrdinaryEmbeddableStruct = (this.TypeKind == TypeKind.Struct) && (this.SpecialType == Microsoft.CodeAnalysis.SpecialType.None) && this.ContainingAssembly.IsLinked;

MethodDefinitionHandle? extensionMarkerMethod = _lazyUncommonProperties?.lazyExtensionInfo?.MarkerMethod;
Debug.Assert(extensionMarkerMethod is not null || this.TypeKind is not TypeKind.Extension);

try
{
foreach (var methodHandle in module.GetMethodsOfTypeOrThrow(_handle))
{
if (isOrdinaryEmbeddableStruct || module.ShouldImportMethod(_handle, methodHandle, moduleSymbol.ImportOptions))
if (isOrdinaryEmbeddableStruct || module.ShouldImportMethod(_handle, methodHandle, moduleSymbol.ImportOptions) ||
extensionMarkerMethod == methodHandle)
{
var method = new PEMethodSymbol(moduleSymbol, this, methodHandle);
members.Add(method);
Expand Down
22 changes: 19 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,26 +378,42 @@ protected void CheckEffectiveAccessibility(TypeWithAnnotations returnType, Immut
}
}

if (!IsStatic && this.GetIsNewExtensionMember() && ContainingType.ExtensionParameter is { } extensionParameter)
Copy link
Member

@jjonescz jjonescz Mar 19, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Checking this.GetIsNewExtensionMember() seems redundant if we have ContainingType.ExtensionParameter is { }

(also applies to similar place in SourcePropertySymbol.cs) #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 19, 2025

Choose a reason for hiding this comment

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

Checking this.GetIsNewExtensionMember() seems redundant if we have ContainingType.ExtensionParameter is { }

It might appear this way. However, strictly speaking, ContainingSymbol (which is behind GetIsNewExtensionMember check) and ContainingType aren't always the same symbol. But after GetIsNewExtensionMember returns true, we know that they are.

{
if (!extensionParameter.TypeWithAnnotations.IsAtLeastAsVisibleAs(this, ref useSiteInfo))
Copy link
Member

@jjonescz jjonescz Mar 19, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Consider combining the nested ifs into one condition with && #Pending

{
// Inconsistent accessibility: parameter type '{1}' is less accessible than method '{0}'
diagnostics.Add(code, GetFirstLocation(), this, extensionParameter.Type);
}
}

diagnostics.Add(GetFirstLocation(), useSiteInfo);
}

protected void CheckFileTypeUsage(TypeWithAnnotations returnType, ImmutableArray<ParameterSymbol> parameters, BindingDiagnosticBag diagnostics)
{
if (ContainingType.HasFileLocalTypes())
NamedTypeSymbol containingType = ContainingType;

if (containingType is { IsExtension: true, ContainingType: { } enclosing })
{
containingType = enclosing;
}

if (containingType.HasFileLocalTypes())
{
return;
}

if (returnType.Type.HasFileLocalTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, GetFirstLocation(), returnType.Type, ContainingType);
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, GetFirstLocation(), returnType.Type, containingType);
}

foreach (var param in parameters)
{
if (param.Type.HasFileLocalTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, GetFirstLocation(), param.Type, ContainingType);
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, GetFirstLocation(), param.Type, containingType);
}
}
}
Expand Down
29 changes: 25 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,18 @@ private TypeWithAnnotations ComputeType(Binder binder, SyntaxNode syntax, Bindin
diagnostics.Add((this.IsIndexer ? ErrorCode.ERR_BadVisIndexerReturn : ErrorCode.ERR_BadVisPropertyType), Location, this, type.Type);
}

if (type.Type.HasFileLocalTypes() && !ContainingType.HasFileLocalTypes())
if (type.Type.HasFileLocalTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Location, type.Type, ContainingType);
NamedTypeSymbol containingType = ContainingType;
if (containingType is { IsExtension: true, ContainingType: { } enclosing })
{
containingType = enclosing;
}

if (!containingType.HasFileLocalTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Location, type.Type, containingType);
}
}

diagnostics.Add(Location, useSiteInfo);
Expand Down Expand Up @@ -652,22 +661,34 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,

var useSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(diagnostics, ContainingAssembly);

var containingTypeForFileTypeCheck = this.ContainingType;
if (containingTypeForFileTypeCheck is { IsExtension: true, ContainingType: { } enclosing })
{
containingTypeForFileTypeCheck = enclosing;
}

foreach (ParameterSymbol param in Parameters)
{
if (!IsExplicitInterfaceImplementation && !this.IsNoMoreVisibleThan(param.Type, ref useSiteInfo))
{
diagnostics.Add(ErrorCode.ERR_BadVisIndexerParam, Location, this, param.Type);
}
else if (param.Type.HasFileLocalTypes() && !this.ContainingType.HasFileLocalTypes())
else if (param.Type.HasFileLocalTypes() && !containingTypeForFileTypeCheck.HasFileLocalTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Location, param.Type, this.ContainingType);
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Location, param.Type, containingTypeForFileTypeCheck);
}
else if (SetMethod is object && param.Name == ParameterSymbol.ValueParameterName)
{
diagnostics.Add(ErrorCode.ERR_DuplicateGeneratedName, param.TryGetFirstLocation() ?? Location, param.Name);
}
}

if (!IsStatic && this.GetIsNewExtensionMember() && ContainingType.ExtensionParameter is { } extensionParameter &&
!this.IsNoMoreVisibleThan(extensionParameter.Type, ref useSiteInfo))
{
diagnostics.Add(ErrorCode.ERR_BadVisIndexerParam, Location, this, extensionParameter.Type);
}

diagnostics.Add(Location, useSiteInfo);

if (IsPartialDefinition && OtherPartOfPartial is { } implementation)
Expand Down
Loading