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 expression loophole: ability to invoke .Add() method with the parameter type not matching the collection expression element type #7724

Open
controlflow opened this issue Oct 29, 2023 · 9 comments
Assignees
Labels

Comments

@controlflow
Copy link

controlflow commented Oct 29, 2023

Version Used: main branch

Steps to Reproduce:

using System.Collections;
using System.Collections.Generic;

BadCandidateForCollectionExpression xs = [123]; // no .Add(int)
BadCandidateForCollectionExpression ys = ["abc"]; // not int element type
BadCandidateForCollectionExpression zs = [default]; // ????

class BadCandidateForCollectionExpression : IEnumerable<int>
{
  public void Add(string s) { }

  IEnumerator<int> IEnumerable<int>.GetEnumerator() { yield break; }
  IEnumerator IEnumerable.GetEnumerator() { yield break; }
}

Expected Behavior:

Impossible to fill the collection expression, no Add method can accept the value of int type - the element type inferred as a collection expression element type.

Actual Behavior:

If the expression is both convertible to inferred element type and can be accepted by some non-int Add method overload - it compiles successfully.
As the result you have a really weird language context - you can only pass expressions that are implicitly convertible to int, but the invoked method accepts string value.

I guess this is the poorly written check in frontend, I guess instead of checking the element expression to be convertible to int you probably must check the parameter type of invoked .Add() method to have type convertible to int.

@controlflow controlflow changed the title Collection expression loophole Collection expression loophole: ability to invoke .Add() method with the parameter type not matching the collection expression element type Oct 29, 2023
@CyrusNajmabadi
Copy link
Member

@cston @333fred to see if this is by design based on our last meetings

I don't imagine this is an interesting case to support given that it doesn't behave like any normal collection does.

@333fred
Copy link
Member

333fred commented Oct 30, 2023

This doesn't seem like it should compile. The type of default should be inferred to be the element type.

@jcouv
Copy link
Member

jcouv commented Oct 30, 2023

I believe this is by-design from current spec. There exists a conversion from default to the iteration type int and default can also be used to invoke .Add. Those two checks are independent (the types don't have to match).

@RikkiGibson
Copy link
Contributor

feels like a spec bug :)

@jaredpar
Copy link
Member

Moving to csharplang to close the spec bug.

@jaredpar jaredpar transferred this issue from dotnet/roslyn Nov 28, 2023
@Ai-N3rd
Copy link
Contributor

Ai-N3rd commented Mar 4, 2024

This should not be supported unless we can find a reasonable use case. Otherwise, It shouldn't be supported for collection expressions.

@GeirGrusom
Copy link

A behavior I found a bit amusing here is that if you change the parameter into a float, and you try to pass a float to that method it will complain that it will only allow an int, not a float! If you give it an int however, it will silently pass a float instead.

@Ai-N3rd
Copy link
Contributor

Ai-N3rd commented Mar 22, 2024

There should probably be a check at compile time. It would simply check if the .Add() method has the same type as the IEnumerable. If not, it can't be used in collections expressions.

@cston
Copy link
Member

cston commented Mar 22, 2024

There should probably be a check at compile time.

Perhaps a diagnostic could be reported if the element as converted for the Add call is not implicitly convertible to the collection iteration type.

BadCandidateForCollectionExpression zs = [default]; // warning: converting 'default' to 'string' for Add(string)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants