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.

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

Implement scoping and shadowing rules for extension parameter and type parameters #77704

Merged
merged 5 commits into from
Mar 21, 2025
Merged
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -183,7 +183,6 @@ public override Binder VisitMethodDeclaration(MethodDeclarationSyntax methodDecl
{
method = method ?? GetMethodSymbol(methodDecl, resultBinder);
isIteratorBody = method.IsIterator;
resultBinder = WithExtensionReceiverParameterBinderIfNecessary(resultBinder, method);
resultBinder = new InMethodBinder(method, resultBinder);
}

@@ -194,19 +193,6 @@ public override Binder VisitMethodDeclaration(MethodDeclarationSyntax methodDecl
return resultBinder;
}

private static Binder WithExtensionReceiverParameterBinderIfNecessary(Binder resultBinder, MethodSymbol method)
{
if (method is { IsStatic: false, ContainingType: SourceNamedTypeSymbol { IsExtension: true, ExtensionParameter: { } parameter } })
{
// PROTOTYPE: Depending on whether we consider method parameters and receiver parameter in the same scope and
// what are the name conflict/shadowing rules, we might consider to adjust behavior of InMethodBinder instead.
// If we decide to keep usage of WithParametersBinder, we might want to update XML doc comment for it.
return new WithParametersBinder([parameter], resultBinder);
}

return resultBinder;
}

public override Binder VisitConstructorDeclaration(ConstructorDeclarationSyntax parent)
{
// If the position isn't in the scope of the method, then proceed to the parent syntax node.
@@ -327,7 +313,6 @@ public override Binder VisitAccessorDeclaration(AccessorDeclarationSyntax parent

if ((object)accessor != null)
{
resultBinder = WithExtensionReceiverParameterBinderIfNecessary(resultBinder, accessor);
resultBinder = new InMethodBinder(accessor, resultBinder);

resultBinder = resultBinder.SetOrClearUnsafeRegionIfNecessary(
@@ -437,7 +422,6 @@ private Binder VisitPropertyOrIndexerExpressionBody(BasePropertyDeclarationSynta
// `isIteratorBody` to `SetOrClearUnsafeRegionIfNecessary` above.
Debug.Assert(!accessor.IsIterator);

resultBinder = WithExtensionReceiverParameterBinderIfNecessary(resultBinder, accessor);
resultBinder = new InMethodBinder(accessor, resultBinder);
}

@@ -797,6 +781,11 @@ internal Binder VisitTypeDeclarationCore(TypeDeclarationSyntax parent, NodeUsage
{
resultBinder = new WithClassTypeParametersBinder(typeSymbol, resultBinder);
}

if (typeSymbol.IsExtension)
{
resultBinder = new WithExtensionParameterBinder(typeSymbol, resultBinder);
}
}
}

8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
@@ -2116,6 +2116,14 @@ private BoundExpression BindNonMethod(SimpleNameSyntax node, Symbol symbol, Bind
{
Error(diagnostics, ErrorCode.ERR_InvalidPrimaryConstructorParameterReference, node, parameter);
}
else if (parameter.ContainingSymbol is NamedTypeSymbol { IsExtension: true } &&
(InParameterDefaultValue || InAttributeArgument ||
this.ContainingMember() is not { Kind: not SymbolKind.NamedType, IsStatic: false } || // We are not in an instance member
(object)this.ContainingMember().ContainingSymbol != parameter.ContainingSymbol) &&
Copy link
Member

@jcouv jcouv Mar 20, 2025

Choose a reason for hiding this comment

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

(object)this.ContainingMember().ContainingSymbol != parameter.ContainingSymbol

Is this condition necessary? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 20, 2025

Choose a reason for hiding this comment

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

That is for references in nested types. IsInDeclaringTypeInstanceMember does the same. Declaring a nested type in extension is an error, but I prefer to be consistent

Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a test for this

!IsInsideNameof)
{
Error(diagnostics, ErrorCode.ERR_InvalidExtensionParameterReference, node, parameter);
}
else
{
// Records never capture parameters within the type
37 changes: 29 additions & 8 deletions src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs
Original file line number Diff line number Diff line change
@@ -30,10 +30,10 @@ internal void ValidateParameterNameConflicts(
bool allowShadowingNames,
BindingDiagnosticBag diagnostics)
{
PooledHashSet<string>? tpNames = null;
PooledDictionary<string, TypeParameterSymbol>? tpNames = null;
if (!typeParameters.IsDefaultOrEmpty)
{
tpNames = PooledHashSet<string>.GetInstance();
tpNames = PooledDictionary<string, TypeParameterSymbol>.GetInstance();
foreach (var tp in typeParameters)
{
var name = tp.Name;
@@ -42,7 +42,7 @@ internal void ValidateParameterNameConflicts(
continue;
}

if (!tpNames.Add(name))
if (!tpNames.TryAdd(name, tp))
{
// Type parameter declaration name conflicts are detected elsewhere
}
@@ -65,16 +65,37 @@ internal void ValidateParameterNameConflicts(
continue;
}

if (tpNames != null && tpNames.Contains(name))
if (tpNames != null && tpNames.TryGetValue(name, out TypeParameterSymbol? tp))
{
// CS0412: 'X': a parameter or local variable cannot have the same name as a method type parameter
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, GetLocation(p), name);
if (tp.ContainingSymbol is NamedTypeSymbol { IsExtension: true })
{
if (p.ContainingSymbol != (object)tp.ContainingSymbol) // Otherwise, SynthesizedExtensionMarker is going to report an error about this conflict
{
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsExtensionTypeParameter, GetLocation(p), name);
}
}
else if (p.ContainingSymbol is NamedTypeSymbol { IsExtension: true })
{
diagnostics.Add(ErrorCode.ERR_TypeParameterSameNameAsExtensionParameter, tp.GetFirstLocationOrNone(), name);
}
else
{
// CS0412: 'X': a parameter or local variable cannot have the same name as a method type parameter
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, GetLocation(p), name);
}
}

if (!pNames.Add(name))
{
// The parameter name '{0}' is a duplicate
diagnostics.Add(ErrorCode.ERR_DuplicateParamName, GetLocation(p), name);
if (parameters[0] is { ContainingSymbol: NamedTypeSymbol { IsExtension: true }, Name: var receiverName } && receiverName == name)
Copy link
Member

@jcouv jcouv Mar 21, 2025

Choose a reason for hiding this comment

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

&& receiverName == name

Consider adding a test to cover this part of the condition (extension(int p1) void M(int p2, int p2)) #Resolved

{
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsExtensionParameter, GetLocation(p), name);
}
else
{
// The parameter name '{0}' is a duplicate
diagnostics.Add(ErrorCode.ERR_DuplicateParamName, GetLocation(p), name);
}
}
else if (!allowShadowingNames)
{
22 changes: 20 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs
Original file line number Diff line number Diff line change
@@ -292,8 +292,16 @@ private static bool ReportConflictWithParameter(Symbol parameter, Symbol newSymb
{
case SymbolKind.Parameter:
case SymbolKind.Local:
// CS0412: '{0}': a parameter, local variable, or local function cannot have the same name as a method type parameter
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, newLocation, name);
if (parameter.ContainingSymbol is NamedTypeSymbol { IsExtension: true })
{
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsExtensionTypeParameter, newLocation, name);
}
else
{
// CS0412: '{0}': a parameter, local variable, or local function cannot have the same name as a method type parameter
diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, newLocation, name);
}

return true;

case SymbolKind.Method:
@@ -324,6 +332,16 @@ internal override bool EnsureSingleDefinition(Symbol symbol, string name, Locati
var parameters = _methodSymbol.Parameters;
var typeParameters = _methodSymbol.TypeParameters;

if (_methodSymbol.GetIsNewExtensionMember())
{
typeParameters = _methodSymbol.ContainingType.TypeParameters.Concat(typeParameters);

if (_methodSymbol.ContainingType.ExtensionParameter is { Name: not "" } receiver)
{
parameters = parameters.Insert(0, receiver);
}
}

if (parameters.IsEmpty && typeParameters.IsEmpty)
{
return false;
Original file line number Diff line number Diff line change
@@ -62,5 +62,19 @@ internal override void AddLookupSymbolsInfoInSingleBinder(LookupSymbolsInfo resu
}
}
}

protected override LookupOptions LookupMask
{
get
{
if (_namedType.IsExtension)
{
// Extension type parameters should get the same treatment as method type parameters
return WithMethodTypeParametersBinder.MethodTypeParameterLookupMask;
}

return base.LookupMask;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
{
/// <summary>
/// Binder used to place extension parameter, if any, in scope.
/// </summary>
internal sealed class WithExtensionParameterBinder : Binder
{
private readonly NamedTypeSymbol _type;

internal WithExtensionParameterBinder(NamedTypeSymbol type, Binder next)
: base(next)
{
_type = type;
}

internal override void AddLookupSymbolsInfoInSingleBinder(LookupSymbolsInfo result, LookupOptions options, Binder originalBinder)
Copy link
Member

@jcouv jcouv Mar 21, 2025

Choose a reason for hiding this comment

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

We probably need some direct LookupSymbols tests to cover this method #Resolved

{
if (options.CanConsiderMembers())
{
if (_type.ExtensionParameter is { Name: not "" } parameter &&
originalBinder.CanAddLookupSymbolInfo(parameter, options, result, null))
{
result.AddSymbol(parameter, parameter.Name, 0);
}
}
}

internal override void LookupSymbolsInSingleBinder(
LookupResult result, string name, int arity, ConsList<TypeSymbol> basesBeingResolved, LookupOptions options, Binder originalBinder, bool diagnose, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
Debug.Assert(result.IsClear);

if ((options & (LookupOptions.NamespaceAliasesOnly | LookupOptions.NamespacesOrTypesOnly)) != 0)
Copy link
Member

@jcouv jcouv Mar 21, 2025

Choose a reason for hiding this comment

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

| LookupOptions.NamespacesOrTypesOnly

I didn't follow, what's the reasoning for checking some options early, but letting others be handled by CheckViability?
For comparison, InMethodBinder and WithLambdaParametersBinder only bail out early for NamespaceAliasesOnly. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 21, 2025

Choose a reason for hiding this comment

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

I didn't follow, what's the reasoning for checking some options early, but letting others be handled by

Why do the work if we know that parameters do not satisfy the request. Especially, when we are binding types names in source, we want to avoid having to create symbols for members, etc. This helps with avoiding cycles. In terms of implementation, I used WithPrimaryConstructorParametersBinder as a template.

{
return;
}

if (_type.ExtensionParameter is { Name: not "" } parameter && parameter.Name == name)
{
result.MergeEqual(originalBinder.CheckViability(parameter, arity, options, null, diagnose, ref useSiteInfo));
}
}
}
}
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal sealed class WithMethodTypeParametersBinder : WithTypeParametersBinder
{
internal const LookupOptions MethodTypeParameterLookupMask = LookupOptions.NamespaceAliasesOnly | LookupOptions.MustNotBeMethodTypeParameter;
private readonly MethodSymbol _methodSymbol;
private MultiDictionary<string, TypeParameterSymbol> _lazyTypeParameterMap;

@@ -57,7 +58,7 @@ protected override LookupOptions LookupMask
{
get
{
return LookupOptions.NamespaceAliasesOnly | LookupOptions.MustNotBeMethodTypeParameter;
return MethodTypeParameterLookupMask;
}
}

Original file line number Diff line number Diff line change
@@ -21,7 +21,6 @@ internal WithTypeParametersBinder(Binder next)
// TODO: Change this to a data structure that won't allocate enumerators
protected abstract MultiDictionary<string, TypeParameterSymbol> TypeParameterMap { get; }

// This is only overridden by WithMethodTypeParametersBinder.
protected virtual LookupOptions LookupMask
{
get
24 changes: 24 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
@@ -8092,4 +8092,28 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExtensionResolutionFailed" xml:space="preserve">
<value>'{0}' does not contain a definition for '{1}' and no accessible extension member '{1}' for receiver of type '{0}' could be found (are you missing a using directive or an assembly reference?)</value>
</data>
<data name="ERR_ReceiverParameterSameNameAsTypeParameter" xml:space="preserve">
<value>'{0}': a receiver parameter cannot have the same name as an extension container type parameter</value>
</data>
<data name="ERR_LocalSameNameAsExtensionTypeParameter" xml:space="preserve">
<value>'{0}': a parameter, local variable, or local function cannot have the same name as an extension container type parameter</value>
</data>
<data name="ERR_TypeParameterSameNameAsExtensionTypeParameter" xml:space="preserve">
<value>Type parameter '{0}' has the same name as an extension container type parameter</value>
</data>
<data name="ERR_LocalSameNameAsExtensionParameter" xml:space="preserve">
<value>'{0}': a parameter, local variable, or local function cannot have the same name as an extension parameter</value>
Copy link
Member

@jcouv jcouv Mar 20, 2025

Choose a reason for hiding this comment

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

an extension parameter

Why "an extension parameter" as opposed to "the extension parameter"?
Also applies to other messages #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the following message as a template "'{0}': a parameter, local variable, or local function cannot have the same name as a method type parameter"

</data>
<data name="ERR_ValueParameterSameNameAsExtensionParameter" xml:space="preserve">
<value>'value': an automatically-generated parameter name conflicts with an extension parameter name</value>
Copy link
Member

@jjonescz jjonescz Mar 21, 2025

Choose a reason for hiding this comment

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

automatically-generated parameter

I think the language term is "implicit parameter". Same comment for ERR_ValueParameterSameNameAsExtensionTypeParameter. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The terminology was "copied" from ERR_DuplicateGeneratedName

</data>
<data name="ERR_TypeParameterSameNameAsExtensionParameter" xml:space="preserve">
<value>Type parameter '{0}' has the same name as an extension parameter</value>
</data>
<data name="ERR_InvalidExtensionParameterReference" xml:space="preserve">
<value>Cannot use extension parameter '{0}' in this context.</value>
</data>
<data name="ERR_ValueParameterSameNameAsExtensionTypeParameter" xml:space="preserve">
<value>'value': an automatically-generated parameter name conflicts with an extension type parameter name</value>
</data>
</root>
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
@@ -2376,6 +2376,14 @@ internal enum ErrorCode
ERR_ExtensionParameterDisallowsDefaultValue = 9503,
ERR_ReceiverParameterOnlyOne = 9504,
ERR_ExtensionResolutionFailed = 9505,
ERR_ReceiverParameterSameNameAsTypeParameter = 9506,
ERR_LocalSameNameAsExtensionTypeParameter = 9507,
ERR_TypeParameterSameNameAsExtensionTypeParameter = 9508,
ERR_LocalSameNameAsExtensionParameter = 9509,
ERR_ValueParameterSameNameAsExtensionParameter = 9510,
ERR_TypeParameterSameNameAsExtensionParameter = 9511,
ERR_InvalidExtensionParameterReference = 9512,
ERR_ValueParameterSameNameAsExtensionTypeParameter = 9513,

// Note: you will need to do the following after adding errors:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
@@ -2493,6 +2493,14 @@ or ErrorCode.ERR_BadExtensionContainingType
or ErrorCode.ERR_ExtensionParameterDisallowsDefaultValue
or ErrorCode.ERR_ReceiverParameterOnlyOne
or ErrorCode.ERR_ExtensionResolutionFailed
or ErrorCode.ERR_ReceiverParameterSameNameAsTypeParameter
or ErrorCode.ERR_LocalSameNameAsExtensionTypeParameter
or ErrorCode.ERR_TypeParameterSameNameAsExtensionTypeParameter
or ErrorCode.ERR_LocalSameNameAsExtensionParameter
or ErrorCode.ERR_ValueParameterSameNameAsExtensionParameter
or ErrorCode.ERR_TypeParameterSameNameAsExtensionParameter
or ErrorCode.ERR_InvalidExtensionParameterReference
or ErrorCode.ERR_ValueParameterSameNameAsExtensionTypeParameter
=> false,
};
#pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.
Original file line number Diff line number Diff line change
@@ -72,7 +72,15 @@ protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymb
diagnostics.Add(ErrorCode.ERR_ReceiverParameterOnlyOne, parameterList.Parameters[parameterIndex].GetLocation());
}

return ParameterHelpers.MakeExtensionReceiverParameter(withTypeParametersBinder: signatureBinder, owner: this, parameterList, diagnostics);
ParameterSymbol? parameter = ParameterHelpers.MakeExtensionReceiverParameter(withTypeParametersBinder: signatureBinder, owner: this, parameterList, diagnostics);

if (parameter is { Name: var name } && name != "" &&
ContainingType.TypeParameters.Any(static (p, name) => p.Name == name, name))
{
diagnostics.Add(ErrorCode.ERR_ReceiverParameterSameNameAsTypeParameter, parameter.GetFirstLocation(), name);
}

return parameter;
}
}
}
Original file line number Diff line number Diff line change
@@ -210,13 +210,25 @@ private static ImmutableArray<TParameterSymbol> MakeParameters<TParameterSyntax,
var methodOwner = owner as MethodSymbol;
var typeParameters = (object?)methodOwner != null ?
methodOwner.TypeParameters :
default(ImmutableArray<TypeParameterSymbol>);
[];

ImmutableArray<ParameterSymbol> parametersForNameConflict = parameters.Cast<TParameterSymbol, ParameterSymbol>();

if (owner.GetIsNewExtensionMember())
{
typeParameters = owner.ContainingType.TypeParameters.Concat(typeParameters);

if (owner.ContainingType.ExtensionParameter is { Name: not "" } receiver)
{
parametersForNameConflict = parametersForNameConflict.Insert(0, receiver);
}
}

Debug.Assert(methodOwner?.MethodKind != MethodKind.LambdaMethod);
bool allowShadowingNames = withTypeParametersBinder.Compilation.IsFeatureEnabled(MessageID.IDS_FeatureNameShadowingInNestedFunctions) &&
methodOwner?.MethodKind == MethodKind.LocalFunction;

withTypeParametersBinder.ValidateParameterNameConflicts(typeParameters, parameters.Cast<TParameterSymbol, ParameterSymbol>(), allowShadowingNames, diagnostics);
withTypeParametersBinder.ValidateParameterNameConflicts(typeParameters, parametersForNameConflict, allowShadowingNames, diagnostics);
}

return parameters;
Loading