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

Collection expressions: require Add method callable with a single argument for class and struct types that do not use [CollectionBuilder] #72654

Merged
merged 19 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -5270,16 +5270,6 @@ public void Add(string s) { }
{
OutputKind = OutputKind.DynamicallyLinkedLibrary,
},
FixedState =
{
ExpectedDiagnostics =
{
// /0/Test0.cs(6,26): error CS1503: Argument 1: cannot convert from 'object' to 'string'
DiagnosticResult.CompilerError("CS1503").WithSpan(6, 26, 6, 36).WithArguments("1", "object", "string"),
// /0/Test0.cs(6,26): error CS9215: Collection expression type must have an applicable instance or extension method 'Add' that can be called with an argument of iteration type 'object'. The best overloaded method is 'MyCollection.Add(string)'.
DiagnosticResult.CompilerError("CS9215").WithSpan(6, 26, 6, 36).WithArguments("object", "MyCollection.Add(string)"),
}
}
}.RunAsync();
}

Expand Down
106 changes: 99 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1285,21 +1285,112 @@ static bool bindInvocationExpressionContinued(
}
}

internal bool HasCollectionExpressionAddMethod(SyntaxNode syntax, TypeSymbol targetType)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

HasCollectionExpressionAddMethod

I am not sure how easy it will be to come up with a scenario making importance of this obvious, but I think this helper should keep track of all dependencies (ref assemblies) that were involved on the path to success and make the caller aware of that set. I do not think the set must be explicitly narrowed, i.e. no reason to trim dependencies from "failed" attempts. Our primary concern is to not drop dependencies for "success". #Closed

{
const string methodName = "Add";
var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;

var lookupResult = LookupResult.GetInstance();
LookupInstanceMember(lookupResult, targetType, leftIsBaseReference: false, rightName: methodName, rightArity: 0, invoked: true, ref useSiteInfo);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

LookupInstanceMember

Rather than trying to duplicate the lookup code, I think we should use Binder.BindInstanceMemberAccess and proceed only if we get a BoundMethodGroup back. I am thinking of a call similar to the one in HasCollectionExpressionApplicableAddMethod.makeInvocationExpression. Afterwards, I think we should pay attention to the BoundMethodGroup.SearchExtensionMethods value before looking for extensions. #Closed

bool anyApplicable = lookupResult.IsMultiViable &&
lookupResult.Symbols.Any(s => s is MethodSymbol m && isApplicableAddMethod(m, expectingExtensionMethod: false));
lookupResult.Free();
if (anyApplicable)
{
return true;
}

var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };

I assume the logic starting from here to the end of the method corresponds to what is happening in MethodGroupResolution Binder.BindExtensionMethod. Consider extracting this code into a local function, adding a comment referring to the "prototype" API (Binder.BindExtensionMethod), adding a comment to Binder.BindExtensionMethod stating that relevant changes there should be mirrored in the local function here. This will make it easier to make sure that necessary adjustments will be made to properly handle extension types. #Closed

foreach (var scope in new ExtensionMethodScopes(this))
{
var methodGroup = MethodGroup.GetInstance();
PopulateExtensionMethodsFromSingleBinder(scope, methodGroup, syntax, implicitReceiver, rightName: methodName, typeArgumentsWithAnnotations: default, BindingDiagnosticBag.Discarded);
anyApplicable = methodGroup.Methods.Any(m => isApplicableAddMethod(m, expectingExtensionMethod: true));
methodGroup.Free();
if (anyApplicable)
{
return true;
}
}

return false;

static bool isApplicableAddMethod(MethodSymbol method, bool expectingExtensionMethod)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

isApplicableAddMethod

We probably should pay attention to use-site diagnostics (ignore methods with errors), and make consumers of HasCollectionExpressionAddMethod aware of them in case of a failure. #Closed

{
if (method.IsStatic != expectingExtensionMethod)
{
return false;
}

var parameters = method.Parameters;
int valueIndex = expectingExtensionMethod ? 1 : 0;
int requiredLength = valueIndex + 1;
if (parameters.Length < requiredLength)
{
return false;
}

// Any trailing parameters must be optional or params.
for (int i = requiredLength; i < parameters.Length; i++)
{
if (parameters[i] is { IsOptional: false, IsParams: false })
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

IsParams

The IsParams check is inconsistent with how overload resolution does that.

  • Only value on corresponding least overridden method matters
  • Only value on the last parameter matters.
  • The definition type of the least overridden method parameter matters, see OverloadResolution.IsValidParams. In fact I suggest to use that helper here #Closed

{
return false;
}
}

// Value parameter must be by value, in, or ref readonly.
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

// Value parameter must be by value, in, or ref readonly.

Are there ref restrictions for optional parameters? #Closed

if (parameters[valueIndex].RefKind is not (RefKind.None or RefKind.In or RefKind.RefReadOnlyParameter))
{
return false;
}

// If the method is generic, can the type arguments be inferred from the one or two arguments?
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

can the type arguments be inferred

I assume we can talk only about a possibility of inference. The check below doesn't really guarantee that the inference is going to succeed. #Closed

var typeParameters = method.TypeParameters;
if (typeParameters.Length > 0)
{
var usedParameterTypes = parameters.Slice(0, requiredLength).SelectAsArray(p => p.Type);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

SelectAsArray

What is the result type of Slice? Do we really need to allocate an array? Could we perhaps use an ArrayBuilder from a pool? #Closed

Copy link
Member Author

@cston cston Mar 25, 2024

Choose a reason for hiding this comment

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

Changed to AsSpan(0, requiredLength).

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 22, 2024

Choose a reason for hiding this comment

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

p => p.Type

The projection doesn't look necessary, we should be able to get to the type from ParameterSymbol right when it is needed #Closed

foreach (var typeParameter in typeParameters)
{
if (!usedParameterTypes.Any((parameterType, typeParameter) => parameterType.ContainsTypeParameter(typeParameter), typeParameter))
{
// The type parameter does not appear in any of the parameter types.
return false;
}
}
}

return true;
}
}

/// <summary>
/// If the element is from a collection type where elements are added with collection initializers,
/// return the argument to the collection initializer Add method or null if the element is not a
/// collection initializer node. Otherwise, return the element as is.
/// </summary>
internal static BoundExpression? GetUnderlyingCollectionExpressionElement(BoundCollectionExpression expr, BoundExpression? element)
internal static BoundExpression GetUnderlyingCollectionExpressionElement(BoundCollectionExpression expr, BoundExpression element, bool throwOnErrors)
{
if (expr.CollectionTypeKind is CollectionExpressionTypeKind.ImplementsIEnumerable)
{
return element switch
switch (element)
{
BoundCollectionElementInitializer collectionInitializer => getCollectionInitializerElement(collectionInitializer),
BoundDynamicCollectionElementInitializer dynamicInitializer => dynamicInitializer.Arguments[0],
_ => null,
};
case BoundCollectionElementInitializer collectionInitializer:
return getCollectionInitializerElement(collectionInitializer);
case BoundDynamicCollectionElementInitializer dynamicInitializer:
return dynamicInitializer.Arguments[0];
}
if (throwOnErrors)
{
throw ExceptionUtilities.UnexpectedValue(element);
}
switch (element)
{
case BoundCall call:
return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0];
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2024

Choose a reason for hiding this comment

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

return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0];

This logic looks suspicious. It is not obvious what are we trying to accomplish and why. It looks like BoundCollectionExpression holds onto the original UnconvertedCollectionExpression node. Could we. perhaps, use information from that node instead? Not sure what do we want to do with expressions that do not have a natural type though. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like it would be better to place this special logic into CreateBoundCollectionExpressionSpreadElement, where it would make sense as an error recovery logic. It doesn't feel like it belongs in this general helper because we don't really know where the element node is coming from and whether it is appropriate to dig into it this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

This case is to unwrap the Add() argument when bindCollectionInitializerElementAddMethod fails in overload resolution with one or more inapplicable or ambiguous Add methods. We hit this case for spreads and for regular elements.

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 26, 2024

Choose a reason for hiding this comment

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

We hit this case for spreads and for regular elements.

Thanks for the clarification. Consider adding a comment what the code is trying to accomplish and why it makes sense to do what it is doing.

default:
return element;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 26, 2024

Choose a reason for hiding this comment

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

return element;

I assume this code is reachable. Therefore, the error recovery logic feels at least inaccurate. For example, it feels like the previous case can unwrap a call that actually was an underlying collection expression element. #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.

My expectation is the BoundCall case above is only used for Add calls, and all other error cases should be BoundBadExpression only. I've added an assert for BoundBadExpression to this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Strengthened checks based on offline discussion.

}
}
return element;

Expand Down Expand Up @@ -1422,8 +1513,9 @@ private void GenerateImplicitConversionErrorForCollectionExpression(
}

if (elements.Length > 0 &&
!HasCollectionExpressionApplicableAddMethod(node.Syntax, targetType, elementType, addMethods: out _, diagnostics))
!HasCollectionExpressionAddMethod(node.Syntax, targetType))
{
Error(diagnostics, ErrorCode.ERR_CollectionExpressionMissingAdd_New, node.Syntax, targetType);
reportedErrors = true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ protected override Conversion GetCollectionExpressionConversion(
}

if (elements.Length > 0 &&
!_binder.HasCollectionExpressionApplicableAddMethod(syntax, targetType, elementType, addMethods: out _, BindingDiagnosticBag.Discarded))
!_binder.HasCollectionExpressionAddMethod(syntax, targetType))
{
return Conversion.NoConversion;
}
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6865,6 +6865,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CollectionExpressionMissingAdd" xml:space="preserve">
<value>Collection expression type must have an applicable instance or extension method 'Add' that can be called with an argument of iteration type '{0}'. The best overloaded method is '{1}'.</value>
</data>
<data name="ERR_CollectionExpressionMissingAdd_New" xml:space="preserve">
<value>Collection expression type '{0}' must have an instance or extension method 'Add' that can be called with a single argument.</value>
</data>
<data name="ERR_CollectionBuilderAttributeInvalidType" xml:space="preserve">
<value>The CollectionBuilderAttribute builder type must be a non-generic class or struct.</value>
</data>
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2301,6 +2301,7 @@ internal enum ErrorCode
ERR_ParamsCollectionMissingConstructor = 9228,

ERR_NoModifiersOnUsing = 9229,
ERR_CollectionExpressionMissingAdd_New = 9230, // PROTOTYPE: Replace ERR_CollectionExpressionMissingAdd.

#endregion

Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2431,6 +2431,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.ERR_ParamsCollectionExtensionAddMethod:
case ErrorCode.ERR_ParamsCollectionMissingConstructor:
case ErrorCode.ERR_NoModifiersOnUsing:
case ErrorCode.ERR_CollectionExpressionMissingAdd_New:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele
if (element is BoundCollectionExpressionSpreadElement spreadElement)
{
Debug.Assert(spreadElement.IteratorBody is { });
var iteratorBody = Binder.GetUnderlyingCollectionExpressionElement(node, ((BoundExpressionStatement)spreadElement.IteratorBody).Expression);
var iteratorBody = Binder.GetUnderlyingCollectionExpressionElement(node, ((BoundExpressionStatement)spreadElement.IteratorBody).Expression, throwOnErrors: true);
Debug.Assert(iteratorBody is { });
return spreadElement.Update(
spreadElement.Expression,
Expand All @@ -131,7 +131,7 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele
}
else
{
var result = Binder.GetUnderlyingCollectionExpressionElement(node, (BoundExpression)element);
var result = Binder.GetUnderlyingCollectionExpressionElement(node, (BoundExpression)element, throwOnErrors: true);
Debug.Assert(result is { });
return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1257,13 +1257,14 @@ private IOperation CreateBoundCollectionExpressionElement(BoundCollectionExpress
{
return element is BoundCollectionExpressionSpreadElement spreadElement ?
CreateBoundCollectionExpressionSpreadElement(expr, spreadElement) :
Create(Binder.GetUnderlyingCollectionExpressionElement(expr, (BoundExpression)element) ?? element);
Create(Binder.GetUnderlyingCollectionExpressionElement(expr, (BoundExpression)element, throwOnErrors: false));
}

private ISpreadOperation CreateBoundCollectionExpressionSpreadElement(BoundCollectionExpression expr, BoundCollectionExpressionSpreadElement element)
{
var iteratorBody = ((BoundExpressionStatement?)element.IteratorBody)?.Expression;
var iteratorItem = Binder.GetUnderlyingCollectionExpressionElement(expr, iteratorBody);
var iteratorItem = element.IteratorBody is { } iteratorBody ?
Binder.GetUnderlyingCollectionExpressionElement(expr, ((BoundExpressionStatement)iteratorBody).Expression, throwOnErrors: false) :
null;
var collection = Create(element.Expression);
SyntaxNode syntax = element.Syntax;
bool isImplicit = element.WasCompilerGenerated;
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading