Roslyn: Test plan for "Extension GetEnumerator"

[jcouv]

Language proposal: https://github.com/dotnet/csharplang/issues/3194
General compiler test plan: https://github.com/dotnet/roslyn/blob/master/docs/contributing/Compiler%20Test%20Plan.md

Spec:

• [x] Complete proposal spec is checked into csharplang

Compiler (Both sync and async for each of these):

• [x] [x] LangVer (TestGetEnumeratorPatternViaExtensionsCSharp8, )
• [x] [x] Feature spec exists and reviewed with LDM
• [x] [x] Update compiler test plan
• [x] [x] IOperation/CFG
  
  
  
  • [x] [x] CFG shows the extension method getting called
  
  
• [x] [ ] GetForEachStatementInfo
• [x] [x] Differing access modifiers
  
  
  
  • [x] [x] Extension GetEnumerator should work for internal, unlike regular GetEnumerator (TestGetEnumeratorPatternViaInternalExtensions, TestGetAsyncEnumeratorPatternViaInternalExtensions)
  
  
  • [x] [x] Test with internal instance method _and_ extension method on the same type (TestGetEnumeratorPatternViaExtensionWithInternalInstanceGetEnumerator, TestGetAsyncEnumeratorPatternViaExtensionWithInternalInstanceGetAsyncEnumerator)
  
  
  • [x] [x] private extension methods when inside a static class (TestGetEnumeratorPatternViaAccessiblePrivateExtension, TestGetAsyncEnumeratorPatternViaAccessiblePrivateExtension)
  
  
• [x] [x] out/ref disallowed.
  
  
  
  • [x] [x] out (, )
  
  
  • [x] [x] ref (TestGetEnumeratorPatternViaRefExtensionOnNonAssignableVariable, TestGetAsyncEnumeratorPatternViaExtensionWithParams)
  
  
• [x] [x] in allowed in the same places as Deconstruct. (TestGetEnumeratorPatternViaInExtensionOnNonAssignableVariable, TestGetAsyncEnumeratorPatternViaInExtensionOnAssignableVariable)
• [x] [x] Default parameters (TestGetEnumeratorPatternViaExtensionWithOptionalParameter, TestGetAsyncEnumeratorPatternViaExtensionWithOptionalParameter)
• [x] [x] params arrays (TestGetEnumeratorPatternViaExtensionWithParams, TestGetAsyncEnumeratorPatternViaExtensionWithParams)
• [x] [x] __arglist parameter (TestGetEnumeratorPatternViaExtensionWithArgList, TestGetAsyncEnumeratorPatternViaExtensionWithArgList)
• [x] [x] dynamic prefers existing attempt to call instance GetEnumerator, not an extension method, or does not work on async (TestPreferIEnumeratorInterfaceOnDynamicThanViaExtension, TestCannotUseExtensionGetAsyncEnumeratorOnDynamic)
• [x] [x] Conversions
  
  
  
  • [x] [x] Reference conversions (TestGetEnumeratorPatternViaExtensionsWithUpcast, TestGetAsyncEnumeratorPatternViaExtensionsWithUpcast)
  
  
  • [x] [x] Boxing conversion (TestGetEnumeratorPatternViaExtensionsWithNullableValueTypeConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithNullableValueTypeConversion)
  
  
  • [x] [x] Unboxing (should show helpful error, like for regular invocation) (TestGetEnumeratorPatternViaExtensionsWithUnboxingConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithUnboxingConversion)
  
  
  • [x] [x] Literal 0 to enum (same as Unboxing) (TestGetEnumeratorPatternViaExtensionsWithZeroToEnumConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithZeroToEnumConversion)
  
  
  • [x] [x] Unconstrained generics (TestGetEnumeratorPatternViaExtensionsWithUnconstrainedGenericConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithUnconstrainedGenericConversion)
  
  
  • [x] [x] Constrained generics (TestGetEnumeratorPatternViaExtensionsWithConstrainedGenericConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithConstrainedGenericConversion)
  
  
  • [x] [x] Interpolated string conversions (IFormattableString vs object) (TestGetEnumeratorPatternViaExtensionsWithFormattableStringConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithFormattableStringConversion1)
  
  
  • [x] [x] User-defined conversions (should be invalid) (TestGetEnumeratorPatternViaExtensionsWithUserDefinedImplicitConversion, TestGetAsyncEnumeratorPatternViaExtensionsWithUserDefinedImplicitConversion)
  
  
  • [x] [x] Tuple type conversions (TestGetEnumeratorPatternViaExtensionsOnTupleWithNestedConversions, TestGetAsyncEnumeratorPatternViaExtensionsOnTupleWithNestedConversions)
  
  
• [x] [x] this types:
  
  
  
  • [x] [x] class (a lot)
  
  
  • [x] [x] interface (TestGetEnumeratorPatternViaExtensionsOnInterface, TestGetAsyncEnumeratorPatternViaExtensionsOnInterface)
  
  
  • [x] [x] delegate (TestGetEnumeratorPatternViaExtensionsOnDelegate, TestGetAsyncEnumeratorPatternViaExtensionsOnDelegate)
  
  
  • [x] [x] struct (, TestGetAsyncEnumeratorPatternOnRange)
  
  
  • [x] [x] enum (TestGetEnumeratorPatternViaExtensionsOnEnum, TestGetAsyncEnumeratorPatternViaExtensionsOnEnum)
  
  
  • [x] [x] nullable (TestGetEnumeratorPatternViaExtensionsOnNullable, TestGetAsyncEnumeratorPatternViaExtensionsOnNullable)
  
  
  • [x] [x] type parameters (TestGetEnumeratorPatternViaExtensionsOnTypeParameter, TestGetAsyncEnumeratorPatternViaExtensionsOnTypeParameter)
  
  
• [x] [x] Ambiguous overloads (TestGetEnumeratorPatternViaAmbiguousExtensions, TestGetAsyncEnumeratorPatternViaAmbiguousExtensions)
• [x] [x] Correct name signature, but return doesn't fulfill the pattern (, TestMoveNextAsyncPatternViaExtensions1)
• [x] [x] Extension is preferred last (TestPreferEnumeratorPatternFromInstanceThanViaExtension, TestPreferAsyncEnumeratorPatternFromInstanceThanViaExtension)
• [x] [x] If GetEnumerator is defined on the class, extensions are not searched, even if return is incorrect. (TestPreferEnumeratorPatternFromInstanceThanViaExtensionEvenWhenInvalid, TestPreferAsyncEnumeratorPatternFromInstanceThanViaExtensionEvenWhenInvalid)
• [x] [x] Multiple extensions in different namespace levels
  
  
  
  • [x] [x] Valid overload in closest namespace (with invalid in outer namespace) (TestGetEnumeratorPatternViaValidExtensionInClosestNamespaceInvalidInFurtherNamespace1-2, TestGetAsyncEnumeratorPatternViaValidExtensionInClosestNamespaceInvalidInFurtherNamespace1-2)
  
  
  • [x] [x] Invalid overload in closest namespace (with valid in outer namespace) (TestGetEnumeratorPatternViaInvalidExtensionInClosestNamespaceValidInFurtherNamespace1-2, TestGetAsyncEnumeratorPatternViaInvalidExtensionInClosestNamespaceValidInFurtherNamespace1-2)
  
  
• [x] [x] ref returns (TestGetEnumeratorPatternViaExtensionWithRefReturn, TestGetAsyncEnumeratorPatternViaExtensionWithRefReturn)
• [x] [x] Nullable warnings
  
  
  
  • [x] [x] No warning for extension method accepting null (ForEach_ExtensionGetEnumerator6, ForEach_ExtensionGetAsyncEnumerator6)
  
  
  • [x] [x] Warning for extension method not accepting null (ForEach_ExtensionGetEnumerator1, ForEach_ExtensionGetAsyncEnumerator5)
  
  
  • [x] [x] Constraint mismatch warnings (ForEach_ExtensionGetEnumerator3, ForEach_ExtensionGetAsyncEnumerator3)
  
  
  • [x] [x] Warning for nullable return (ForEach_ExtensionGetEnumerator15, ForEach_ExtensionGetAsyncEnumerator15)
  
  
  • [x] [x] Output attributes are honored after call (ForEach_ExtensionGetEnumerator7-8, ForEach_ExtensionGetAsyncEnumerator7)
  
  
• [x] [x] Obsolete extension methods (TestGetEnumeratorPatternViaObsoleteExtension, TestWithObsoletePatternMethodsViaExtension)
• Disposal
  
  
  
  • [x] [x] GetEnumerator() returning IDisposable types (TestForEachViaExtensionExplicitlyDisposableStruct, TestAwaitForEachViaExtensionExplicitlyDisposableStruct)
  
  
  • [x] [x] Uses pattern-based Dispose method on async, not on sync (TestForEachViaExtensionDisposeStruct, TestAwaitForEachViaExtensionAsyncDisposeStruct)
  
  
  • [x] [x] Uses pattern-based for both for ref structs (, TestAwaitForEachViaExtensionImplicitlyDisposableStruct)
  
  

Productivity:

• [ ] [ ] test FindAllReferences
• [x] [x] AddParameter to a GetEnumerator
• [ ] [ ] AddUsing to bring extension GetEnumerator into scope
• [ ] [ ] foreach to Linq
• [ ] [ ] Linq to foreach

[333fred]

@YairHalberstadt I've updated the test plan. I haven't yet gone through the existing tests to check the boxes, and we'll need to do a compiler review of the plan to see what's blocking. I'll try to go through the existing tests either this afternoon or tomorrow so we have an idea of what's outstanding.

[333fred]

@YairHalberstadt I've filled in the checkboxes. First check is for synchronous, second is for async.

[YairHalberstadt]

Thanks. Going through them 1 by 1. Will comment here as I have issues:

It looks like __arglist isn't allowed currently for an instance GetAsyncEnumerator either.

[YairHalberstadt]

Nullable (value type) conversions

Extension methods on a nullable value type don't work on a non-nullable valuetype and vice versa.

Therefore I would expect extension foreach to fail in both cases.

[YairHalberstadt]

Unboxing (should show helpful error, like for regular invocation)

Do you mean an extension method on int, when you the expression is object?

[333fred]

It looks like __arglist isn't allowed currently for an instance GetAsyncEnumerator either.

That's correct. Many of these are error scenarios, we just need to make sure we don't crash.

Extension methods on a nullable value type don't work on a non-nullable valuetype and vice versa.
Therefore I would expect extension foreach to fail in both cases.

Yep. We do have a good error message for the regular extension method case, I would expect a similar message here.

Do you mean an extension method on int, when you the expression is object?

From int? to int, for example.

[333fred]

I removed the Nullable (value type) conversion bullet, I meant to do so after I broke it up into boxing and unboxing and forgot.

[YairHalberstadt]

Actually, for an instance GetEnumerator, we unwrap nullable value types. SHould we do the same for extension GetEnumerator?

[YairHalberstadt]

Interpolated string conversions (IFormattableString vs object)

This goes straight to foreach on a string (which is what we want)

[YairHalberstadt]

Do you want to add delegate conversion to the test plan?

[YairHalberstadt]

As a result of the nullable unwrapping this doesn't work:

using System;
public class C
{
    public static void Main()
    {
        foreach (var i in (int?)null)
        {
            Console.Write(i);
        }
    }
    public sealed class Enumerator
    {
        public int Current { get; private set; }
        public bool MoveNext() => Current++ != 3;
    }
}
public static class Extensions
{
    public static C.Enumerator GetEnumerator(this int? self) => new C.Enumerator();
}

Is this what we want?

[333fred]

Do you want to add delegate conversion to the test plan?

Do you mean method group/lambda to delegate type?

As a result of the nullable unwrapping this doesn't work:

using System;
public class C
{
    public static void Main()
    {
        foreach (var i in (int?)null)
        {
            Console.Write(i);
        }
    }
    public sealed class Enumerator
    {
        public int Current { get; private set; }
        public bool MoveNext() => Current++ != 3;
    }
}
public static class Extensions
{
    public static C.Enumerator GetEnumerator(this int? self) => new C.Enumerator();
}

Is this what we want?

No. I would expect that example to work, given that you can do ((int?)null).GetEnumerator().

[YairHalberstadt]

But we should still unwrap nullable types? I'm guessing this will require two passes of overload resolution, one with the type and one with the unwrapped type.

[YairHalberstadt]

Although even then I'm not sure that's correct. Which of them would win if the extension method on the nullable type is in a further namespace than the extension method on the unwrapped type?

[333fred]

But we should still unwrap nullable types? I'm guessing this will require two passes of overload resolution, one with the type and one with the unwrapped type.

You mean should we unbox the nullable type? Absolutely not. You can't call an extension method defined on int on a value of type int? today, and I wouldn't expect that to change. We _should_ allow implicit boxing conversions, but unboxing is never implicit.

[YairHalberstadt]

@333fred

The following currently works:

public struct Program {
    public static void Main() {
        foreach (var a in (Program?)null)
        {
        }
    }

    public IEnumerator<int> GetEnumerator() => throw null;
}

So it makes sense for instance members and extension members to be consistent in this regard.

[YairHalberstadt]

https://github.com/dotnet/roslyn/blob/d04ebc2e1e0f0738998bd0806dcb19114adb683c/src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs#L576-L578

// This behavior is not spec'd, but it's what Dev10 does.

[333fred]

https://github.com/dotnet/roslyn/blob/d04ebc2e1e0f0738998bd0806dcb19114adb683c/src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs#L576-L578

// This behavior is not spec'd, but it's what Dev10 does.

Given that text, I'd rather not take what is effectively a "the native compiler did this weird thing so let's replicate" into a new feature. I'll double check with the LDM on this, but I'd expect the rules for calling an extension method to be followed.

[333fred]

https://github.com/dotnet/roslyn/blob/d04ebc2e1e0f0738998bd0806dcb19114adb683c/src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs#L576-L578

// This behavior is not spec'd, but it's what Dev10 does.

Given that text, I'd rather not take what is effectively a "the native compiler did this weird thing so let's replicate" into a new feature. I'll double check with the LDM on this, but I'd expect the rules for calling an extension method to be followed.

We're not going to replicate the behavior. This should behave the same as regular extension methods.

[YairHalberstadt]

Another case I've discovered:

Any object known to be null is invalid in a foreach (e.g. default(IEnumerable)), unless the type is a nullable value type (e.g. default(ImmutableArray<int>?) is allowed).

I'm going to make an exception to this when we use an extension GetEnumerator, since they may be able to handle nulls.

[YairHalberstadt]

Implementation of this testplan ongoing at https://github.com/dotnet/roslyn/pull/45160

[YairHalberstadt]

GetSymbolInfo/LookupSymbols/GetMethodGroup work correctly

Can you show some examples of where we have these tests in the codebase

[YairHalberstadt]

AddParameter to a GetEnumerator
foreach to Linq
Linq to foreach

What am I testing for in each of these cases?

[333fred]

GetSymbolInfo/LookupSymbols/GetMethodGroup work correctly

Can you show some examples of where we have these tests in the codebase

Look for tests that use the GetForEachStatementInfo API.

AddParameter to a GetEnumerator
foreach to Linq
Linq to foreach

What am I testing for in each of these cases?

Generally speaking, that they work. For AddParameter, we should just make sure it doesn't crash, really. For the other two, I would expect them to work.

[YairHalberstadt]

AddUsing to bring extension GetEnumerator into scope

PR available at https://github.com/dotnet/roslyn/pull/45348

Seperate PR since it's IDE and a separate feature.

[YairHalberstadt]

For the other two, I would expect them to work.

I'm not sure that makes much sense. Extension GetEnumerator only applies if somethings not IEnumerable, and hence it will not be convertible to linq. Similiarly linq to foreach will never require use of extension GetEnumerator since it will implement IEnumerable.

[YairHalberstadt]

I've added a test for find all references for extension GetEnumerator. I discovered that find all references doesn't work at all for GetAsyncEnumerator. I've opened an issue for that (https://github.com/dotnet/roslyn/issues/45352) and will hopefully deal with it at some point, but I presume it is not critical for now.

All I've got left to do are the following:

GetSymbolInfo/LookupSymbols/GetMethodGroup work correctly
AddParameter to a GetEnumerator
foreach to Linq
Linq to foreach

Though as I said above, I'm not sure what there is to test in foreach to linq/linq to foreach

I think it's probably worth starting to review my PR now.

[333fred]

@YairHalberstadt I've updated the test plan with new cases from test plan review, and I'll go through the latest tests in the branch next to make sure the status is fully up to date.

[333fred]

@YairHalberstadt I've updated the test plan. It looks like we're in very good shape, just another couple of tests to add:

• I didn't see any explicit tests for GetForEachStatementInfo.
• I didn't see any tests with out as a extension method receiver. Not sure this one is actually important, given that you can't have out as a this parameter anyway.
• TestForEachViaExtensionImplicitlyDisposableStruct doesn't actually test what it says it does, and I believe the scenario is covered by TestForEachViaExtensionDisposeStruct anyway.
• This comment can be removed now that we have a decision: https://github.com/dotnet/roslyn/blob/features/extension-foreach/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenForEachTests.cs#L4127-L4128
• The feature needs to be added to https://github.com/dotnet/roslyn/blob/features/extension-foreach/docs/contributing/Compiler%20Test%20Plan.md.

[YairHalberstadt]

I didn't see any explicit tests for GetForEachStatementInfo.

The pattern for all existing ForEach tests is to call GetBoundForEachStatement which internally calls and tests GetForEachStatementInfo. I added some tests which do this.

[YairHalberstadt]

https://github.com/dotnet/roslyn/pull/46299

[333fred]

The pattern for all existing ForEach tests is to call GetBoundForEachStatement which internally calls and tests GetForEachStatementInfo. I added some tests which do this.

@YairHalberstadt I see this for the sync version, is there an async version I'm missing?

[333fred]

Test plan is complete and the feature is merged.