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 1 commit
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
Next Next commit
Collection expressions: require Add method callable with single argum…
…ent for class and struct types that do not use [CollectionBuilder]
  • Loading branch information
cston committed Mar 22, 2024
commit 442498d5305ed1b66d1eef56d2f8866f52fac1f4
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
82 changes: 81 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,85 @@ 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
Expand Down Expand Up @@ -1422,8 +1501,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
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.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.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.ru.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.tr.xlf

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

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

Loading
Loading