Extensions: Address follow-ups related to collection expressions by jcouv · Pull Request #79877 · dotnet/roslyn

jcouv · 2025-08-12T07:09:24Z

Closes #78960
The first commit addresses minor post-PR-merge follow-ups from last PR: #79702 (comment)

The design needs to be confirmed with LDM, it is outlined here and copied below:

#### Collection expressions

- Extension `Add` works
- Extension `GetEnumerator` works for spread
- Extension `GetEnumerator` does not affect the determination of the element type (must be instance)
- Static `Create` extension methods should not count as a blessed **create** method
- Should extension countable properties affect collection expressions?

#### `params` collections

- Extensions `Add` does not affect what types are allowed with `params`

Note: the last LDM decision regarding pattern-based constructs was to not let extension properties count as "countable properties" in any of the relevant scenarios (list-patterns and implicit indexers) so I kept that behavior here as well.

Relates to test plan #76130

jcouv · 2025-08-12T07:25:04Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+                    {
+                        if (member.TryGetCorrespondingExtensionImplementationMethod() is { } extensionImplementation)
+                        {
+                            member = extensionImplementation;


📝 since we swap the implementation method for the member here and reduce it below, I had to relax the assertion in TypeMap again (see PR)
An alternative would be to implement the logic for this method specially for new extension methods, instead of using the implementation method to leverage the existing handling

AlekseyTs · 2025-08-13T02:25:40Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

                {
                    // If the method is generic, skip it if the type arguments cannot be inferred.
                    var member = candidate.Member;
+                    if (member.GetIsNewExtensionMember())


if (member.GetIsNewExtensionMember())

I think this special handling deserves comment. #Closed

AlekseyTs · 2025-08-13T02:29:36Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

                builder: out var builder);
            // Collection expression target types require instance method GetEnumerator.
-            if (result && builder.ViaExtensionMethod) // Tracked by https://github.com/dotnet/roslyn/issues/78960: Add test coverage for new extensions
+            if (result && builder.ViaExtensionMethod)


ViaExtensionMethod

Is this flag set when a new extension is used? #Closed

Yes, see getEnumeratorInfo

if (SatisfiesGetEnumeratorPattern(syntax, collectionSyntax, ref builder, collectionExpr, isAsync, viaExtensionMethod: true, diagnostics)) { return createPatternBasedEnumeratorResult(ref builder, collectionExpr, isAsync, viaExtensionMethod: true, diagnostics); }

The relevant test is CollectionExpression_09

AlekseyTs · 2025-08-13T02:35:14Z

src/Compilers/CSharp/Portable/Symbols/SymbolEqualityComparer.cs

            {
                return ConsiderEverything;
            }
-            else if (comparison == TypeCompareKind.AllIgnoreOptions)


else if (comparison == TypeCompareKind.AllIgnoreOptions)

I see no harm in keeping this case. Should we switch to a switch, BTW. #Closed

I prefer to remove since not exercised

AlekseyTs · 2025-08-13T02:40:02Z

src/Compilers/CSharp/Portable/Symbols/TypeMap.cs

        {
            // mapping contents are read-only hereafter
-            Debug.Assert(allowAlpha || !from.Any(static tp => tp is SubstitutedTypeParameterSymbol));
+            Debug.Assert(allowAlpha || !from.Any(static tp => tp is SubstitutedTypeParameterSymbol && tp.ContainingSymbol is not SourceExtensionImplementationMethodSymbol));


tp.ContainingSymbol is not SourceExtensionImplementationMethodSymbol

I think it would be better to check if the type parameter is original definition. Those should be fine to substitute regardless what the container is and the check would be more robust long term. #Closed

AlekseyTs · 2025-08-13T02:44:30Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs

            (type, parameter, unused) => type.TypeKind == TypeKind.TypeParameter && (parameter is null || TypeSymbol.Equals(type, parameter, TypeCompareKind.ConsiderEverything2));

-        public static bool ContainsTypeParameter(this TypeSymbol type, MethodSymbol parameterContainer)
+        public static bool ContainsTypeParameter(this TypeSymbol type, Symbol parameterContainer)


parameterContainer

Consider renaming to "typeParameterContainer". Here and in s_isTypeParameterWithSpecificContainerPredicate initialization #Closed

AlekseyTs · 2025-08-13T02:52:56Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

-                // (7,26): error CS1929: 'MyCollection<object>' does not contain a definition for 'Add' and the best extension method overload 'E.extension<T>(MyCollection<T>).Add(T)' requires a receiver of type 'MyCollection<T>'
-                // MyCollection<object> c = [oNull, oNotNull];
-                Diagnostic(ErrorCode.ERR_BadInstanceArgType, "[oNull, oNotNull]").WithArguments("MyCollection<object>", "Add", "E.extension<T>(MyCollection<T>).Add(T)", "MyCollection<T>").WithLocation(7, 26));
+        comp.VerifyEmitDiagnostics();


comp.VerifyEmitDiagnostics();

Consider executing and observing the result #Closed

AlekseyTs · 2025-08-13T03:01:05Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+    }
+}
+
+public class D : System.Collections.IEnumerable


D

Is this type used? #Closed

AlekseyTs · 2025-08-13T03:02:14Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+{
+    extension(C c)
+    {
+        public int Length => 2;


2

I assume the purpose of the test is to verify that this property isn't used. Let's throw from it then. #Closed

AlekseyTs · 2025-08-13T03:05:53Z

Done with review pass (commit 2)

jjonescz · 2025-08-13T12:13:53Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+}
+""";
+        var comp = CreateCompilationWithIL(src, ilSrc);
+        comp.VerifyEmitDiagnostics();


Should we verify execution to check that E2 was used, not E? #Resolved

jjonescz · 2025-08-13T12:16:25Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+""";
+        var comp = CreateCompilation(src);
+        comp.VerifyEmitDiagnostics(
+            // (1,7): error CS1921: The best overloaded method match for 'E.extension(C).Add(int)' has wrong signature for the initializer element. The initializable Add must be an accessible instance method.


This error wording seems inaccurate - the Add can be an instance or extension method. But I guess that's a pre-existing issue. #WontFix

Yup. I'll leave as pre-existing issue

AlekseyTs

LGTM (commit 4)

Use CLRSignatureCompareOptions (follow-up from collisions PR)

eaeace3

jcouv self-assigned this Aug 12, 2025

jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Aug 12, 2025

jcouv commented Aug 12, 2025

View reviewed changes

jcouv force-pushed the extension-collections branch from 01c628c to 083fbaa Compare August 12, 2025 07:27

jcouv mentioned this pull request Jun 25, 2025

Extensions: nullability follow-ups #78828

Open

1 task

Extensions: Address follow-ups related to collection expressions

03e8cbd

jcouv force-pushed the extension-collections branch from 083fbaa to 03e8cbd Compare August 12, 2025 07:31

jcouv mentioned this pull request Aug 6, 2025

Test plan for "extensions" feature #76130

Open

jcouv marked this pull request as ready for review August 12, 2025 15:29

jcouv requested a review from a team as a code owner August 12, 2025 15:29

jcouv requested a review from jjonescz August 12, 2025 15:29

AlekseyTs reviewed Aug 13, 2025

View reviewed changes

Address feedback

861fc88

jjonescz approved these changes Aug 13, 2025

View reviewed changes

Test execution

eb9944c

build-analysis bot mentioned this pull request Aug 13, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

AlekseyTs approved these changes Aug 14, 2025

View reviewed changes

jcouv merged commit 9138704 into dotnet:main Aug 14, 2025
24 checks passed

jcouv deleted the extension-collections branch August 14, 2025 05:26

dotnet-policy-service bot added this to the Next milestone Aug 14, 2025

This was referenced Aug 19, 2025

[Automated] PRs inserted in VS build main-10918.105 #79952

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10918.21 #79956

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

dotnet-bot mentioned this pull request Aug 22, 2025

[Automated] PRs inserted in VS build feature.Wix5-10921.95 #80011

Closed

davidwengier mentioned this pull request Aug 25, 2025

Allow Razor cohosting to work with non-Razor SDK projects dotnet/vscode-csharp#8559

Merged

Conversation

jcouv commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Aug 13, 2025

Uh oh!

jjonescz Aug 13, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Aug 13, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Aug 12, 2025 •

edited

Loading

jcouv Aug 12, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

AlekseyTs Aug 13, 2025 •

edited

Loading

jjonescz Aug 13, 2025 •

edited by jcouv

Loading

jjonescz Aug 13, 2025 •

edited by jcouv

Loading