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

Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

Merged
Show file tree
Hide file tree
Changes from 4 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
Expand Up @@ -6,6 +6,7 @@

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
Expand All @@ -19,6 +20,13 @@ private class QueryTranslationState
// Represents the current translation state for a query. Consider a query of the form
// from ID in EXPR { clauses } SELECT ...

#if DEBUG
/// <summary>
/// For debug assert only
/// </summary>
public string nextInvokedMethodName;
#endif

// EXPR, above
public BoundExpression fromExpression;

Expand Down Expand Up @@ -76,7 +84,7 @@ internal RangeVariableSymbol AddRangeVariable(Binder binder, SyntaxToken identif
}
}

if (!error)
if (!error && (object)diagnostics != BindingDiagnosticBag.Discarded)
{
var collisionDetector = new LocalScopeBinder(binder);
collisionDetector.ValidateDeclarationNameConflictsInScope(result, diagnostics);
Expand Down Expand Up @@ -111,6 +119,11 @@ internal RangeVariableSymbol TransparentRangeVariable(Binder binder)

public void Clear()
{
#if DEBUG
Debug.Assert(nextInvokedMethodName is null);
nextInvokedMethodName = null;
#endif

fromExpression = null;
rangeVariable = null;
selectOrGroup = null;
Expand Down
138 changes: 101 additions & 37 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1498,35 +1498,22 @@ private BoundExpression BindIdentifier(
// It's possible that the argument list is malformed; if so, do not attempt to bind it;
// just use the null array.

int arity = node.Arity;
bool hasTypeArguments = arity > 0;
bool hasTypeArguments = node.Arity > 0;

SeparatedSyntaxList<TypeSyntax> typeArgumentList = node.Kind() == SyntaxKind.GenericName
? ((GenericNameSyntax)node).TypeArgumentList.Arguments
: default(SeparatedSyntaxList<TypeSyntax>);

Debug.Assert(arity == typeArgumentList.Count);
Debug.Assert(node.Arity == typeArgumentList.Count);

var typeArgumentsWithAnnotations = hasTypeArguments ?
BindTypeArguments(typeArgumentList, diagnostics) :
default(ImmutableArray<TypeWithAnnotations>);

var lookupResult = LookupResult.GetInstance();
LookupOptions options = LookupOptions.AllMethodsOnArityZero;
if (invoked)
{
options |= LookupOptions.MustBeInvocableIfMember;
}

if (!IsInMethodBody && !IsInsideNameof)
{
Debug.Assert((options & LookupOptions.NamespacesOrTypesOnly) == 0);
options |= LookupOptions.MustNotBeMethodTypeParameter;
}

var name = node.Identifier.ValueText;
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
this.LookupSymbolsWithFallback(lookupResult, name, arity: arity, useSiteInfo: ref useSiteInfo, options: options);
this.LookupIdentifier(lookupResult, node, invoked, ref useSiteInfo);
diagnostics.Add(node, useSiteInfo);

if (lookupResult.Kind != LookupResultKind.Empty)
Expand Down Expand Up @@ -1657,6 +1644,23 @@ void adjustIdentifierMapIfAny(SimpleNameSyntax node, bool invoked)
#endif
}

private void LookupIdentifier(LookupResult lookupResult, SimpleNameSyntax node, bool invoked, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
LookupOptions options = LookupOptions.AllMethodsOnArityZero;
if (invoked)
{
options |= LookupOptions.MustBeInvocableIfMember;
}

if (!IsInMethodBody && !IsInsideNameof)
{
Debug.Assert((options & LookupOptions.NamespacesOrTypesOnly) == 0);
options |= LookupOptions.MustNotBeMethodTypeParameter;
}

this.LookupSymbolsWithFallback(lookupResult, node.Identifier.ValueText, arity: node.Arity, useSiteInfo: ref useSiteInfo, options: options);
}

/// <summary>
/// Is this is an _ identifier in a context where discards are allowed?
/// </summary>
Expand Down Expand Up @@ -1919,11 +1923,8 @@ private BoundExpression BindNonMethod(SimpleNameSyntax node, Symbol symbol, Bind
var primaryCtor = parameter.ContainingSymbol as SynthesizedPrimaryConstructor;

if (primaryCtor is not null &&
(InParameterDefaultValue ||
InAttributeArgument ||
this.ContainingMember() is not { Kind: not SymbolKind.NamedType, IsStatic: false } contaningMember || // We are not in an instance member
(object)contaningMember.ContainingSymbol != primaryCtor.ContainingSymbol || // The member doesn't belong to our type, i.e. from nested type
((object)contaningMember != primaryCtor && contaningMember is MethodSymbol { MethodKind: MethodKind.Constructor })) && // We are in a non-primary instance constructor
(!IsInDeclaringTypeInstanceMember(primaryCtor) ||
(this.ContainingMember() is MethodSymbol { MethodKind: MethodKind.Constructor } contaningMember && (object)contaningMember != primaryCtor)) && // We are in a non-primary instance constructor
Copy link
Member

@cston cston Jan 6, 2023

Choose a reason for hiding this comment

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

contaning

Typo. #Closed

!IsInsideNameof)
{
Error(diagnostics, ErrorCode.ERR_InvalidPrimaryConstructorParameterReference, node, parameter);
Expand Down Expand Up @@ -2034,6 +2035,14 @@ bool isUsedBeforeDeclaration(SimpleNameSyntax node, LocalSymbol localSymbol)
}
}

private bool IsInDeclaringTypeInstanceMember(SynthesizedPrimaryConstructor primaryCtor)
{
return !(InParameterDefaultValue ||
InAttributeArgument ||
this.ContainingMember() is not { Kind: not SymbolKind.NamedType, IsStatic: false } contaningMember || // We are not in an instance member
Copy link
Member

@cston cston Jan 6, 2023

Choose a reason for hiding this comment

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

contaning

Typo. #Closed

(object)contaningMember.ContainingSymbol != primaryCtor.ContainingSymbol); // The member doesn't belong to our type, i.e. from nested type
}

private bool ReportSimpleProgramLocalReferencedOutsideOfTopLevelStatement(SimpleNameSyntax node, Symbol symbol, BindingDiagnosticBag diagnostics)
{
if (symbol.ContainingSymbol is SynthesizedSimpleProgramEntryPointSymbol &&
Expand Down Expand Up @@ -6288,6 +6297,7 @@ private BoundExpression BindMemberAccess(
BindingDiagnosticBag diagnostics)
{
Debug.Assert(node != null);
Debug.Assert(invoked == SyntaxFacts.IsInvoked(node));

BoundExpression boundLeft;

Expand Down Expand Up @@ -6348,6 +6358,10 @@ private BoundExpression BindLeftOfPotentialColorColorMemberAccess(ExpressionSynt
[MethodImpl(MethodImplOptions.NoInlining)]
private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(IdentifierNameSyntax left, BindingDiagnosticBag diagnostics)
{
Debug.Assert((left.Parent is MemberAccessExpressionSyntax { RawKind: (int)SyntaxKind.SimpleMemberAccessExpression } memberAccess && memberAccess.Expression == left) ||
(left.Parent is QualifiedNameSyntax qualifiedName && qualifiedName.Left == left) ||
(left.Parent is FromClauseSyntax { Parent: QueryExpressionSyntax query } fromClause && query.FromClause == fromClause && fromClause.Expression == left));

// SPEC: 7.6.4.1 Identical simple names and type names
// SPEC: In a member access of the form E.I, if E is a single identifier, and if the meaning of E as
// SPEC: a simple-name (spec 7.6.2) is a constant, field, property, local variable, or parameter with the
Expand Down Expand Up @@ -6390,12 +6404,17 @@ private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(Iden
var boundType = BindNamespaceOrType(left, typeDiagnostics);
if (TypeSymbol.Equals(boundType.Type, leftType, TypeCompareKind.AllIgnoreOptions))
{
Debug.Assert(!leftType.IsDynamic());
Debug.Assert(IsPotentialColorColorReceiver(left, leftType));
Copy link
Member

@cston cston Jan 6, 2023

Choose a reason for hiding this comment

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

Debug.Assert(IsPotentialColorColorReceiver(left, leftType));

What is the assert checking? (It looks like IsPotentialColorColorReceiver() is checking the same two conditions as the two if conditions above.) #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.

What is the assert checking? (It looks like IsPotentialColorColorReceiver() is checking the same two conditions as the two if conditions above.)

It is verifying that we can rely on IsPotentialColorColorReceiver elsewhere to check for the same condition.


// NOTE: ReplaceTypeOrValueReceiver will call CheckValue explicitly.
boundValue = BindToNaturalType(boundValue, valueDiagnostics);
return new BoundTypeOrValueExpression(left,
new BoundTypeOrValueData(leftSymbol, boundValue, valueDiagnostics, boundType, typeDiagnostics), leftType);
}
}

Debug.Assert(!IsPotentialColorColorReceiver(left, leftType));
break;

// case SymbolKind.Event: //SPEC: 7.6.4.1 (a.k.a. Color Color) doesn't cover events
Expand All @@ -6408,6 +6427,14 @@ private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(Iden
return boundValue;
}

private bool IsPotentialColorColorReceiver(IdentifierNameSyntax id, TypeSymbol type)
{
string name = id.Identifier.ValueText;

return (type.Name == name || IsUsingAliasInScope(name)) &&
TypeSymbol.Equals(BindNamespaceOrType(id, BindingDiagnosticBag.Discarded).Type, type, TypeCompareKind.AllIgnoreOptions);
}

// returns true if name matches a using alias in scope
// NOTE: when true is returned, the corresponding using is also marked as "used"
private bool IsUsingAliasInScope(string name)
Expand Down Expand Up @@ -6476,13 +6503,25 @@ private BoundExpression BindDynamicMemberAccess(
hasErrors: hasErrors);
}

#if DEBUG
/// <summary>
/// Bind the RHS of a member access expression, given the bound LHS.
/// It is assumed that CheckValue has not been called on the LHS.
/// </summary>
/// <remarks>
/// If new checks are added to this method, they will also need to be added to
/// <see cref="MakeQueryInvocation(CSharpSyntaxNode, BoundExpression, string, TypeSyntax, TypeWithAnnotations, BindingDiagnosticBag, string)"/>.
/// </remarks>
#else
/// <summary>
/// Bind the RHS of a member access expression, given the bound LHS.
/// It is assumed that CheckValue has not been called on the LHS.
/// </summary>
/// <remarks>
/// If new checks are added to this method, they will also need to be added to <see cref="MakeQueryInvocation(CSharpSyntaxNode, BoundExpression, string, TypeSyntax, TypeWithAnnotations, BindingDiagnosticBag)"/>.
/// If new checks are added to this method, they will also need to be added to
/// <see cref="MakeQueryInvocation(CSharpSyntaxNode, BoundExpression, string, TypeSyntax, TypeWithAnnotations, BindingDiagnosticBag)"/>.
/// </remarks>
#endif
private BoundExpression BindMemberAccessWithBoundLeft(
ExpressionSyntax node,
BoundExpression boundLeft,
Expand Down Expand Up @@ -6820,11 +6859,6 @@ private BoundExpression BindInstanceMemberAccess(
{
Debug.Assert(rightArity == (typeArgumentsWithAnnotations.IsDefault ? 0 : typeArgumentsWithAnnotations.Length));
var leftType = boundLeft.Type;
LookupOptions options = LookupOptions.AllMethodsOnArityZero;
if (invoked)
{
options |= LookupOptions.MustBeInvocableIfMember;
}

var lookupResult = LookupResult.GetInstance();
try
Expand All @@ -6836,13 +6870,8 @@ private BoundExpression BindInstanceMemberAccess(
// UNDONE: Classify E as prop access, indexer access, variable or value

bool leftIsBaseReference = boundLeft.Kind == BoundKind.BaseReference;
if (leftIsBaseReference)
{
options |= LookupOptions.UseBaseReferenceAccessibility;
}

CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
this.LookupMembersWithFallback(lookupResult, leftType, rightName, rightArity, ref useSiteInfo, basesBeingResolved: null, options: options);
this.LookupInstanceMember(lookupResult, leftType, leftIsBaseReference, rightName, rightArity, invoked, ref useSiteInfo);
diagnostics.Add(right, useSiteInfo);

// SPEC: Otherwise, an attempt is made to process E.I as an extension method invocation.
Expand All @@ -6862,6 +6891,13 @@ private BoundExpression BindInstanceMemberAccess(

if (searchExtensionMethodsIfNecessary)
{
BoundExpression colorColorValueReceiver = GetValueExpressionIfTypeOrValueReceiver(boundLeft);

if (IsPossiblyCapturingPrimaryConstructorParameterReference(colorColorValueReceiver, out _))
{
boundLeft = ReplaceTypeOrValueReceiver(boundLeft, useType: false, diagnostics);
}

var boundMethodGroup = new BoundMethodGroup(
node,
typeArgumentsWithAnnotations,
Expand Down Expand Up @@ -6889,6 +6925,22 @@ private BoundExpression BindInstanceMemberAccess(
}
}

private void LookupInstanceMember(LookupResult lookupResult, TypeSymbol leftType, bool leftIsBaseReference, string rightName, int rightArity, bool invoked, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
LookupOptions options = LookupOptions.AllMethodsOnArityZero;
if (invoked)
{
options |= LookupOptions.MustBeInvocableIfMember;
}

if (leftIsBaseReference)
{
options |= LookupOptions.UseBaseReferenceAccessibility;
}

this.LookupMembersWithFallback(lookupResult, leftType, rightName, rightArity, ref useSiteInfo, basesBeingResolved: null, options: options);
}

private void BindMemberAccessReportError(BoundMethodGroup node, BindingDiagnosticBag diagnostics)
{
var nameSyntax = node.NameSyntax;
Expand Down Expand Up @@ -7322,21 +7374,18 @@ private void PopulateExtensionMethodsFromSingleBinder(
BindingDiagnosticBag diagnostics)
{
int arity;
LookupOptions options;
if (typeArgumentsWithAnnotations.IsDefault)
{
arity = 0;
options = LookupOptions.AllMethodsOnArityZero;
}
else
{
arity = typeArgumentsWithAnnotations.Length;
options = LookupOptions.Default;
}

var lookupResult = LookupResult.GetInstance();
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
this.LookupExtensionMethodsInSingleBinder(scope, lookupResult, rightName, arity, options, ref useSiteInfo);
this.LookupExtensionMethods(lookupResult, scope, rightName, arity, ref useSiteInfo);
diagnostics.Add(node, useSiteInfo);

if (lookupResult.IsMultiViable)
Expand All @@ -7354,6 +7403,21 @@ private void PopulateExtensionMethodsFromSingleBinder(
lookupResult.Free();
}

private void LookupExtensionMethods(LookupResult lookupResult, ExtensionMethodScope scope, string rightName, int arity, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
LookupOptions options;
if (arity == 0)
{
options = LookupOptions.AllMethodsOnArityZero;
}
else
{
options = LookupOptions.Default;
}

this.LookupExtensionMethodsInSingleBinder(scope, lookupResult, rightName, arity, options, ref useSiteInfo);
}

protected BoundExpression BindFieldAccess(
SyntaxNode node,
BoundExpression receiver,
Expand Down
21 changes: 21 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,27 @@ private BoundExpression ReplaceTypeOrValueReceiver(BoundExpression receiver, boo
}
}

private BoundExpression GetValueExpressionIfTypeOrValueReceiver(BoundExpression receiver)
Copy link
Member

@cston cston Jan 6, 2023

Choose a reason for hiding this comment

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

private

static #Closed

{
if ((object)receiver == null)
{
return null;
}

switch (receiver)
{
case BoundTypeOrValueExpression typeOrValueExpression:
return typeOrValueExpression.Data.ValueExpression;

case BoundQueryClause queryClause:
// a query clause may wrap a TypeOrValueExpression.
return GetValueExpressionIfTypeOrValueReceiver(queryClause.Value);

default:
return null;
}
}

/// <summary>
/// Return the delegate type if this expression represents a delegate.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Lambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ internal partial class Binder
// If we have no modifiers then the modifiers array is null; if we have any modifiers
// then the modifiers array is non-null and not empty.

internal UnboundLambda AnalyzeAnonymousFunction(
private UnboundLambda AnalyzeAnonymousFunction(
AnonymousFunctionExpressionSyntax syntax, BindingDiagnosticBag diagnostics)
{
// !!! The only binding operations allowed here - binding type references
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal void LookupExtensionMethods(LookupResult result, string name, int arity
/// <remarks>
/// Makes a second attempt if the results are not viable, in order to produce more detailed failure information (symbols and diagnostics).
/// </remarks>
internal Binder LookupSymbolsWithFallback(LookupResult result, string name, int arity, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, ConsList<TypeSymbol> basesBeingResolved = null, LookupOptions options = LookupOptions.Default)
private Binder LookupSymbolsWithFallback(LookupResult result, string name, int arity, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, ConsList<TypeSymbol> basesBeingResolved = null, LookupOptions options = LookupOptions.Default)
{
Debug.Assert(options.AreValid());

Expand Down
Loading