Extensions: Close some tracked follow-ups (using static directives) by jcouv · Pull Request #80527 · dotnet/roslyn

jcouv · 2025-10-01T09:47:21Z

Relates to test plan #76130
Closes #79440

Also added a section on using static directives for extensions: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-14.0/extensions.md#using-static-directives

jcouv · 2025-10-01T09:48:02Z

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

        {
            Debug.Assert(extensions.Count == 0);

-            // Tracked by https://github.com/dotnet/roslyn/issues/79440 : using directives, test this flag (see TestUnusedExtensionMarksImportsAsUsed)


📝 Added TestUnusedExtensionMarksImportsAsUsed_02

jcouv · 2025-10-01T09:48:44Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

                                                            });
                            }

-                            // Tracked by https://github.com/dotnet/roslyn/issues/78957 : public API, Ensure we are not messing up relative order of events for extension members (with relation to events for enclosing types, etc.)


📝 Already covered by AnalyzerActions_ tests

jcouv · 2025-10-01T09:49:41Z

src/Compilers/CSharp/Portable/Emitter/Model/MethodSymbolAdapter.cs

                    return synthesizedGlobalMethod.ContainingPrivateImplementationDetailsType;
                }

-                // Tracked by https://github.com/dotnet/roslyn/issues/78827 : code quality, share logic with Cci.ITypeMemberReference.GetContainingType implementation?


📝 I removed a few similar comments, as I don't think the suggestion is necessary or that it would improve the code.

jcouv · 2025-10-01T10:56:33Z

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

                )
-                // Method begins at RVA 0x20fa
+                // Method begins at RVA 0x20e2
                // Code size 1 (0x1)


📝 Added a dedicated test for GetTypeByMetadataName #Resolved

📝 Added a dedicated test for GetTypeByMetadataName

Did you mean to make this comment on line 44444 which removed this snippet?

// Tracked by https://github.com/dotnet/roslyn/issues/78968 : we should find the unspeakable nested type Assert.Null(comp.GetTypeByMetadataName("E+<G>$BA41CFE2B5EDAEB8C1B9062F59ED4D69"));

And was it actually fixed? The comment seems to indicate that GetTypeByMetadataName should not return null, but the new test still has Assert.Null.

Yes, the comment about GetTypeByMetadataName got misplaced.
Decided not to support it following conversation with Aleksey. Here for context:

jjonescz · 2025-10-03T10:28:18Z

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

                )
-                // Method begins at RVA 0x20fa
+                // Method begins at RVA 0x20e2
                // Code size 1 (0x1)


📝 Added a dedicated test for GetTypeByMetadataName

Did you mean to make this comment on line 44444 which removed this snippet?

// Tracked by https://github.com/dotnet/roslyn/issues/78968 : we should find the unspeakable nested type Assert.Null(comp.GetTypeByMetadataName("E+<G>$BA41CFE2B5EDAEB8C1B9062F59ED4D69"));

And was it actually fixed? The comment seems to indicate that GetTypeByMetadataName should not return null, but the new test still has Assert.Null.

jjonescz · 2025-10-03T10:30:38Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

            }
        }

+        private bool HasValidate(TreeType node)


Seems like this could be static. #Resolved

[Responding to other comment here due to technical difficulties]
Yes, the comment about GetTypeByMetadataName got misplaced.
Decided not to support it following conversation with Aleksey. Here for context:

jjonescz · 2025-10-03T10:30:55Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/TestWriter.cs


 #nullable disable

+using System.Diagnostics;


This added using seems unnecessary. #Resolved

AlekseyTs · 2025-10-03T14:31:01Z

src/Compilers/CSharp/Test/Symbol/Compilation/GetUnusedImportDirectivesTests.cs

+            var syntax = tree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().Single().Expression;
+
+            //This is the crux of the test.
+            //Without this line, with or without the fix, the model never gets pushed to evaluate extension method candidates


with or without the fix

It is not clear what fix we are referring to. It looks like addition of this test corresponds to a removal of a comment in implementation. #Closed

Replaced this with a more compact/focused test (LookupSymbols_UsedImports)

AlekseyTs · 2025-10-03T14:37:54Z

src/Compilers/CSharp/Test/Symbol/Compilation/GetUnusedImportDirectivesTests.cs

+            //and therefore never marked ClassLibrary2 as a used import in consoleApplication.
+            //Without the fix, this call used to result in ClassLibrary2 getting marked as used, after the fix, this call does not
+            //result in changing ClassLibrary2's used status.
+            model.GetMemberGroup(syntax);


model.GetMemberGroup(syntax);

Does the set include a method from ClassLibrary2? Consider asserting the fact to strengthen the test and to make it clearer. #Closed

AlekseyTs · 2025-10-03T14:44:30Z

src/Compilers/CSharp/Portable/Syntax/ParameterSyntax.cs

+        {
+            if (Type is null && Identifier.IsKind(SyntaxKind.None))
+            {
+                throw new System.NotSupportedException(CSharpResources.ParameterRequiresTypeOrIdentifier);


NotSupportedException

It feels like this should be a flavor of an "invalid argument" exception. #Closed

AlekseyTs · 2025-10-03T14:48:15Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

      </PropertyComment>
    </Field>
-    <Field Name="Type" Type="TypeSyntax" Optional="true" Override="true"/>
+    <Field Name="Type" Type="TypeSyntax" Optional="true" RequiredForTest="true" Override="true"/>


RequiredForTest="true"

What effect does this have? #Closed

The generated tests generate nodes. The basic logic is to use null or default for all optional nodes. That means GenerateParameter() produces a node without type or identifier and the generated tests hit the added validation.
This flag says that even though the type is optional, for purpose of test we treat it as non-optional.

AlekseyTs · 2025-10-03T14:55:46Z

src/Compilers/CSharp/Portable/Syntax/ParameterSyntax.cs

+        {
+            if (Type is null && Identifier.IsKind(SyntaxKind.None))
+            {
+                throw new System.NotSupportedException(CSharpResources.ParameterRequiresTypeOrIdentifier);


throw new System.NotSupportedException(CSharpResources.ParameterRequiresTypeOrIdentifier);

Is there any factory method that will always end-up here no matter what? #Closed

Should we either get rid of those, or not enforce the condition?

I reviewed the usages and the only one that always ends up in this situation has been dealt with. It was the internal GenerateParameter() in generated syntax tests.
The other usages are in SyntaxFactory (one overload takes an identifier but no type) and SyntaxGenerator (takes both a type and an identifier).

AlekseyTs · 2025-10-03T15:00:27Z

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

+            AssertEx.Equal(groupingName, extension.ExtensionGroupingName);
+            AssertEx.Equal(markerName, extension.ExtensionMarkerName);
+            Assert.Null(comp.GetTypeByMetadataName($"E+{groupingName}"));
+            Assert.Null(comp.GetTypeByMetadataName($"E+{groupingName}+{markerName}"));


Assert.Null(comp.GetTypeByMetadataName($"E+{groupingName}+{markerName}"));

Consider also testing the following for completeness:

Assert.Null(comp.GetTypeByMetadataName($"E+{markerName}")); ``` #Closed

AlekseyTs · 2025-10-03T15:12:16Z

Done with review pass (commit 4) #Closed

AlekseyTs

LGTM (commit 6)

* upstream/main: Extensions: Close some tracked follow-ups (dotnet#80527) Fix outdated 17.15 to 18.0 (dotnet#80570) Update dependencies from https://github.com/dotnet/dotnet build 285582 (dotnet#80551) Fix all-in-one tests [main] Update dependencies from dotnet/arcade (dotnet#80559) Improve error recovery when object initializer uses ':' instead of '=' (dotnet#80553) Support `field` keyword in EE. (dotnet#80515) Log a debug message for ContentModified exceptions. (dotnet#80549) Update dependencies from https://github.com/dotnet/arcade build 20251002.2 (dotnet#80550)

* upstream/main: (252 commits) Move Watch EA to a separate assembly Microsoft.CodeAnalysis.ExternalAccess.HotReload (dotnet#80556) Enable long paths for Windows DartLab CI (dotnet#80581) Ensure that CS8659 is reported on partial properties (dotnet#80573) Fix a wrong relative link in a doc (dotnet#80567) [main] Source code updates from dotnet/dotnet (dotnet#80578) Update dependencies from https://github.com/dotnet/arcade build 20251006.2 (dotnet#80577) Update main configs after VS snap (dotnet#80523) Add followup async public apis (dotnet#80455) Reduce allocations in CSharpSyntaxNode.GetStructure (dotnet#80562) Extensions: Close some tracked follow-ups (dotnet#80527) Fix outdated 17.15 to 18.0 (dotnet#80570) Update dependencies from https://github.com/dotnet/dotnet build 285582 (dotnet#80551) Fix all-in-one tests [main] Update dependencies from dotnet/arcade (dotnet#80559) Improve error recovery when object initializer uses ':' instead of '=' (dotnet#80553) Support `field` keyword in EE. (dotnet#80515) Log a debug message for ContentModified exceptions. (dotnet#80549) Update dependencies from https://github.com/dotnet/arcade build 20251002.2 (dotnet#80550) Fix Simplify and add additional asserts ...

jcouv self-assigned this Oct 1, 2025

jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Oct 1, 2025

jcouv commented Oct 1, 2025

View reviewed changes

jcouv force-pushed the extensions-misc branch from b07fa87 to 8d048ec Compare October 1, 2025 10:55

jcouv commented Oct 1, 2025

View reviewed changes

jcouv force-pushed the extensions-misc branch from 8d048ec to 2c5cde5 Compare October 1, 2025 10:58

jcouv mentioned this pull request Oct 1, 2025

Extensions: follow-ups related to using directives #79440

Closed

2 tasks

jcouv force-pushed the extensions-misc branch 4 times, most recently from 450cf7d to bb5c1bc Compare October 1, 2025 22:16

Close some tracked follow-ups

3e1ff61

jcouv force-pushed the extensions-misc branch from bb5c1bc to 3e1ff61 Compare October 1, 2025 22:53

jcouv marked this pull request as ready for review October 2, 2025 08:49

jcouv requested a review from a team as a code owner October 2, 2025 08:49

jcouv requested review from AlekseyTs and jjonescz October 3, 2025 09:23

jjonescz reviewed Oct 3, 2025

View reviewed changes

Julien Couvreur added 2 commits October 3, 2025 05:05

Address feedback

cb58b1a

tweak

096f93e

jcouv requested a review from jjonescz October 3, 2025 12:13

jjonescz approved these changes Oct 3, 2025

View reviewed changes

AlekseyTs reviewed Oct 3, 2025

View reviewed changes

Close ordering follow-up

5357f84

AlekseyTs reviewed Oct 3, 2025

View reviewed changes

Address feedback

8946b5b

jcouv requested a review from AlekseyTs October 3, 2025 22:08

Merge remote-tracking branch 'dotnet/main' into extensions-misc

0f79ba6

AlekseyTs approved these changes Oct 6, 2025

View reviewed changes

jcouv merged commit ff59510 into dotnet:main Oct 6, 2025
28 checks passed

jcouv deleted the extensions-misc branch October 6, 2025 22:00

dotnet-policy-service bot added this to the Next milestone Oct 6, 2025

This was referenced Oct 8, 2025

[Automated] PRs inserted in VS build main-11107.64 #80585

Closed

[Automated] PRs inserted in VS build feature.debugger.main-11108.236 #80606

Closed

jcouv mentioned this pull request Oct 17, 2025

Extensions: follow-up parsing issues #78961

Open

5 tasks

jcouv changed the title ~~Extensions: Close some tracked follow-ups~~ Extensions: Close some tracked follow-ups (using static directives) Nov 15, 2025

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026

Conversation

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

Uh oh!

Uh oh!

jcouv Oct 1, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 1, 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!

jjonescz Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 3, 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!

jjonescz Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 3, 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!

AlekseyTs Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 3, 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 Oct 3, 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 Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 3, 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 Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jcouv commented Oct 1, 2025 •

edited

Loading

jcouv Oct 1, 2025 •

edited by AlekseyTs

Loading

jcouv Oct 1, 2025 •

edited

Loading

jjonescz Oct 3, 2025 •

edited

Loading

jjonescz Oct 3, 2025 •

edited

Loading

jjonescz Oct 3, 2025 •

edited by jcouv

Loading

jjonescz Oct 3, 2025 •

edited by jcouv

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs Oct 3, 2025 •

edited

Loading

AlekseyTs commented Oct 3, 2025 •

edited

Loading