Handle infinite cycles through params collection initializers

[jjonescz]

Fixes #74734.

[333fred]

Do we have a work item tracking this?

[jjonescz]

No, I will create one, thanks.

[jjonescz]

On a second thought, maybe it's better to just remove the assert.

[cston]

Base

Minor: All of the tests use a base and derived type. Is that necessary to repro the issue? If not, consider merging the base and derived in at least one test where there is currently only one derived type.

[cston]

Let's also test a generic collection type. Perhaps this existing test could be modified to combine base and derived, and test a generic type:

public class MyCollection1<T> : IEnumerable<T>
{
    public IEnumerator<T> GetEnumerator() => throw null;
    IEnumerator IEnumerable.GetEnumerator() => throw null;
    public void Add(params MyCollection1<T> c) {}
}

public class C
{
    MyCollection1<object> M() => [1];
}

[cston]

Consider testing:

public class MyCollection1<T> : IEnumerable<T>
{
    public IEnumerator<T> GetEnumerator() => throw null;
    IEnumerator IEnumerable.GetEnumerator() => throw null;
    public void Add(params MyCollection1<object> c) {}
}

public class C
{
    MyCollection1<int> M() => [1];
}

[cston]

Should we test overloads, such as the following? (It looks like this should succeed.)

public class MyCollection1<T> : IEnumerable<T>
{
    public IEnumerator<T> GetEnumerator() => throw null;
    IEnumerator IEnumerable.GetEnumerator() => throw null;
    public void Add(T x) {}
    public void Add(params MyCollection1<T> y) {}
}

public class C
{
    MyCollection1<object> M() => [1];
}

[cston]

binder.Syntax.FullSpan.Equals(syntax.FullSpan) &&

Why check spans rather than binder.Syntax == (object)syntax?

[jjonescz]

I'm not sure if the syntax nodes are guaranteed to be the same reference?

[jjonescz]

But it seems it should, and I see other places in roslyn using that, so I will switch to reference equality as well, thanks.

[Spolutrean]

@jjonescz Is it a good idea to also test different call-sites? It is leading to VS crash as well.

public class MyCollection1<T> : IEnumerable<T>
{
    public IEnumerator<T> GetEnumerator() => throw null;
    IEnumerator IEnumerable.GetEnumerator() => throw null;
    public void Add(params MyCollection1<object> c) {}
}

public class C
{
    public void M() 
    {
        MyCollection1<int> c = new() { 1 };
        c.Add(2);
    }
}

[333fred]

I'd still like to understand how we're getting here, as well as having explicit ioperation tests for it.

[jjonescz]

I think we are getting here because of the cycle - the _currentImplicitInstance is unset because the receiver is the same collection expression as the one being visited. I will add tests.

[333fred]

I'm not comfortable with getting rid of this assert entirely. Let's assert the specific case we're talking about, so we still catch any other violations.

[AlekseyTs]

ERR_CollectionInitializerInfiniteChainOfAddCalls

We are handling cycles through constructors (ERR_ParamsCollectionInfiniteChainOfConstructorCalls) elsewhere and with a different approach. I assume that at the time that handling was introduced, there was no way to get into a cycle through an Add method. It feels like we should unify and combine both checks, i.e. do them in the same place and use similar approaches for both. Also, we should confirm that there is no way to get into a cycle when constructors and Add methods are involved at the same time, otherwise we might be missing a guard. #Closed

[jjonescz]

Also, we should confirm that there is no way to get into a cycle when constructors and Add methods are involved at the same time, otherwise we might be missing a guard.

I didn't think of a way that would be possible without hitting the existing guards. I can add some more tests though.

It feels like we should unify and combine both checks, i.e. do them in the same place and use similar approaches for both.

The constructor check is separate and happens before binding the conversion - I considered implementing the Add check at the same place but that would require duplicating the complicated Add binding logic. The other way around (moving the existing constructor check) could be probably done - but it seems orthogonal to this bug fix, perhaps we can just file an issue. Also I think we would need another intermediate binder so it wouldn't share code with the Add check, i.e., maybe there are no advantages to moving the constructor check.

[AlekseyTs]

perhaps we can just file an issue

I think this should be sufficient for now.

[jjonescz]

#75024