Extensions: address some diagnostic quality issues #80827

jcouv · 2025-10-21T00:00:50Z

Addresses parts of #78830

Some diagnostics report extension blocks without a containing type (extension(int) instead of E.extension(int))
Diagnostics for method vs. property or property vs. property conflicts are generic/uninformative

Relates to test plan #76130

jcouv · 2025-10-21T23:48:05Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

                allowTrailingSeparator: false,
                requireOneElement: forExtension, // For extension declarations, we require at least one receiver parameter
                allowSemicolonAsSeparator: false);
-            // Tracked by https://github.com/dotnet/roslyn/issues/78830 : diagnostic quality, consider suppressing parsing diagnostics on extension parameters beyond the first one


📝 I don't this will be that useful

AlekseyTs · 2025-10-22T14:18:27Z

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

                return new MethodGroupResolution(resultSymbol, LookupResultKind.Viable, diagnostics.ToReadOnly());
            }
+
+            static void initActualArguments(BoundExpression left, AnalyzedArguments? analyzedArguments, [NotNull] ref AnalyzedArguments? actualMethodArguments)


initActualArguments

The name feels counterintuitive. We have actual arguments, this method "fakes" an additional argument. Consider dropping "Actual" #Closed

I realized that the "Actual" probably refers to the actualMethodArguments. Both feel confusing, but I guess this is a "pre-existing condition".

AlekseyTs · 2025-10-22T14:22:44Z

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

+                if (methodResult.HasAnyApplicableMethod && propertyResult.HasAnyApplicableMember)
+                {
+                    var firstMethod = methodResult.OverloadResolutionResult?.BestResult.Member ?? methodResult.MethodGroup.Methods[0];
+                    var firstProperty = propertyResult.BestResult.Member;


propertyResult.BestResult.Member

What is this going to produce in case of an ambiguity? #Closed

Good catch. This was broken. BestResult can only be accessed if the overload resolution has succeeded (ie. there's a single result that's still considered applicable, either in normal or expanded form, after overload resolution completes)

AlekseyTs · 2025-10-22T14:28:40Z

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

+                }
+
+                ExtendedErrorTypeSymbol resultSymbol = new ExtendedErrorTypeSymbol(containingSymbol: null, symbols, LookupResultKind.OverloadResolutionFailure, errorInfo, arity);
                return new MethodGroupResolution(resultSymbol, LookupResultKind.Viable, diagnostics.ToReadOnly());


LookupResultKind.Viable

It might be worth explaining why we "call" error result Viable #Closed

Made small adjustment to clarify.
It's meant to flow the kind from the LookupResult, but we can only get here when it's Viable (we have a check for IsMultiViable earlier in ResolveExtensions)

AlekseyTs · 2025-10-22T14:43:16Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

            if (Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypes ||
-                Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces)
+                Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces ||
+                (Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameOnly && symbol.IsExtension))


(Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameOnly && symbol.IsExtension)

I understand that this is probably gives us desired behavior for specific scenarios, but the condition itself is confusing. Especially for someone who doesn't know that we enumerated all possible SymbolDisplayTypeQualificationStyle values that are defined at the moment. I think the condition will have more sense and will be more robust to future changes to the SymbolDisplayTypeQualificationStyle enum if we simply use symbol.IsExtension as the condition. #Closed

AlekseyTs · 2025-10-22T14:57:34Z

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

-            Diagnostic(ErrorCode.ERR_ExtensionResolutionFailed, "C.M").WithArguments("C", "M").WithLocation(1, 22));
+            Diagnostic(ErrorCode.ERR_AmbigExtension, "C.M").WithArguments("E3.extension(C).M()", "E2.extension(C).M").WithLocation(1, 22));
+
+        string expected(int index)


expected

Is this function used? #Closed

AlekseyTs · 2025-10-22T15:51:57Z

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

+                DiagnosticInfo errorInfo;
+                if (methodResult.HasAnyApplicableMethod && propertyResult.HasAnyApplicableMember)
+                {
+                    var firstMethod = methodResult.OverloadResolutionResult?.BestResult.Member ?? methodResult.MethodGroup.Methods[0];


methodResult.MethodGroup.Methods[0]

Is this guaranteed to contain an applicable method? #Closed

This thread was marked as resolved, but it is still not obvious to me why we assume that methodResult.MethodGroup.Methods[0] is an applicable method.

Is there a test that takes this code path?

All the methods in that method group are as applicable as it gets (they meet lookup criteria and are compatible/substitutable given the receiver type).

The scenario where we don't have an overload resolution result of methods is when there are no arguments. For example var x = object.M; or System.Action a = object.M;.
This reporting path is tested by ReportDiagnostics_23, ReportDiagnostics_24 and ReportDiagnostics_25.

In such scenarios, we start with a lookup result (which ensures we match name and arity requirements), then resolveMethods prunes methods that are not compatible with the receiver (using ReduceExtensionMember), instead of running normal overload resolution. We then form a MethodGroupResolution that is considered to have applicable methods (ie. its HasAnyApplicableMethod is true), since there's not possible overload resolution we could run.

// MethodGroupResolution criteria that lets us get here public bool HasAnyApplicableMethod { get { return (this.MethodGroup != null) && (this.ResultKind == LookupResultKind.Viable) && ((this.OverloadResolutionResult == null) || this.OverloadResolutionResult.HasAnyApplicableMember); } }

AlekseyTs

Done with review pass (commit 1)

jcouv · 2025-10-23T04:23:14Z

@dotnet/roslyn-compiler for second review. Thanks

AlekseyTs · 2025-10-23T14:23:10Z

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

+                    return result.BestResult.Member;
+                }
+
+                return result.ResultsBuilder.First(r => r.Result.Kind == MemberResolutionKind.Worse).Member;


return result.ResultsBuilder.First(r => r.Result.Kind == MemberResolutionKind.Worse).Member;

This looks fragile because the logic doesn't look equivalent to OverloadResolutionResult<TMember>.HasAnyApplicableMember. I suggest adding an OverloadResolutionResult<TMember>.FirstApplicableMember API next to OverloadResolutionResult<TMember>.HasAnyApplicableMember with shared logic for picking an applicable member, and using it here. That should help with keeping consistent behavior now and in the future. #Closed

Perhaps that API doesn't have to return the first applicable candidate. It might return the "best" applicable candidate. The main point is that there should be an obvious guarantee that we get something when HasAnyApplicableMember is true.

I'm okay to move the logic closer to HasAnyApplicableMember, but the distinction that is left here is Worse vs. Worst and it is intentional.

Because the result had HasAnyApplicableMember but not Succeeded, we know there's at least one Worse in the set. Worst is worse than Worse

AlekseyTs · 2025-10-23T14:39:49Z

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

+                {
+                    initActualArguments(left, analyzedArguments, ref actualMethodArguments);
+
+                    propertyResult.ReportDiagnostics(binder, expression.Location, expression, diagnostics, memberName, left, left.Syntax, actualMethodArguments, symbols,


actualMethodArguments

This is suspicious. It looks like we are using actualReceiverArguments in resolveProperties. Also, it looks like resolveProperties guarantees that actualReceiverArguments is initialized when propertyResult is not null. We can assert the fact after resolveProperties and on entry into this function. #Closed

Thanks. I failed to noticed that we have separate actual arguments for methods vs. properties, and used the wrong one...

AlekseyTs · 2025-10-23T14:42:06Z

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

+                }
+                else
+                {
+                    initActualArguments(left, analyzedArguments, ref actualMethodArguments);


initActualArguments(left, analyzedArguments, ref actualMethodArguments);

Given the comment about actualReceiverArguments this looks unnecessary. #Closed

AlekseyTs · 2025-10-23T14:43:46Z

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

+                return result.ResultsBuilder.First(r => r.Result.Kind == MemberResolutionKind.Worse).Member;
+            }
+
+            static void initActualArguments(BoundExpression left, AnalyzedArguments? analyzedArguments, [NotNull] ref AnalyzedArguments? actualMethodArguments)


initActualArguments

Given the comment about actualReceiverArguments this helper looks unnecessary, i.e. original code can stay where it was. #Closed

Didn't mean to approve.

AlekseyTs · 2025-10-23T14:51:31Z

Done with review pass (commit 3)

AlekseyTs · 2025-10-23T21:06:27Z

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

+                LookupResult lookupResult,
+                AnalyzedArguments? analyzedArguments,
+                ref AnalyzedArguments? actualMethodArguments,
+                ref AnalyzedArguments? actualReceiverArguments,


ref

AnalyzedArguments is a class and we do not try to change the passed in value, I think we don't need it to be a ref. #Closed

AlekseyTs · 2025-10-23T21:07:36Z

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

+                LookupResult lookupResult,
+                AnalyzedArguments? analyzedArguments,
+                ref AnalyzedArguments? actualMethodArguments,
+                ref AnalyzedArguments? actualReceiverArguments,


AnalyzedArguments? actualReceiverArguments

I think we expect actualReceiverArguments to be not null here #Closed

AlekseyTs · 2025-10-23T21:08:18Z

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

+                string memberName,
+                int arity,
+                LookupResult lookupResult,
+                AnalyzedArguments? analyzedArguments,


AnalyzedArguments? analyzedArguments,

It looks like this parameter isn't used #Closed

AlekseyTs · 2025-10-23T21:08:54Z

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

+                int arity,
+                LookupResult lookupResult,
+                AnalyzedArguments? analyzedArguments,
+                ref AnalyzedArguments? actualMethodArguments,


ref AnalyzedArguments? actualMethodArguments,

It looks like this parameter isn't used #Closed

AlekseyTs · 2025-10-23T21:31:05Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs

+            }
+
+            // We prefer Worse over Worst candidates
+            return ResultsBuilder.First(r => r.Result.Kind == MemberResolutionKind.Worse).Member;


return ResultsBuilder.First(r => r.Result.Kind == MemberResolutionKind.Worse).Member;

We discussed offline using a more defensive logic here. #Closed

AlekseyTs · 2025-10-23T21:34:30Z

Done with review pass (commit 4)

AlekseyTs

LGTM (commit 6), assuming CI is passing

* upstream/main: (332 commits) Cache lambdas in analyzer driver (dotnet#80759) Add information for NuGet package version 4.14 (dotnet#80870) Add missing search keywords to VB Advanced options page Fix IDE0031 false positive when preprocessor directives are used in if statements (dotnet#80878) Use core compiler on netfx hosts with toolset package (dotnet#80631) Make string concat assert more precise (dotnet#80619) Extensions: address some diagnostic quality issues (dotnet#80827) Add note on traversal order for bound nodes (dotnet#80872) Ensure that locals at the top level of a constructor have the same safe-context as parameters (dotnet#80807) Fix handling of SymbolDisplayCompilerInternalOptions.UseArityForGenericTypes option for non-native symbol implementations (dotnet#80826) Update src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests.cs Add IsValidContainingStatement check to prevent collection initializers in using declarations Add back old DocumentSpan constructor (dotnet#80864) Add tests verifying pointer types in type parameters require unsafe context (dotnet#80776) Add regression test for Interlocked.Exchange with nullable types (dotnet#80796) Add regression test for ParseAttributeArgumentList with invalid input (fixes dotnet#8699) (dotnet#80705) Add regression test for compiler crash with syntax error in indexer declaration (dotnet#80772) Add runtime NullReferenceException validation to foreach null iteration tests (dotnet#80839) Update MicrosoftBuildTasksCoreVersionForMetrics to 17.11.48 (dotnet#80812) Mark CS4009 error as a "build only" error. (dotnet#80698) ...

jcouv self-assigned this Oct 21, 2025

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

jcouv force-pushed the extensions-diagnostics branch from c367836 to b706ec5 Compare October 21, 2025 20:33

Extensions: address some diagnostic quality issues

06b2b5a

jcouv force-pushed the extensions-diagnostics branch from b706ec5 to 06b2b5a Compare October 21, 2025 22:26

dotnet-policy-service bot added the VSCode label Oct 21, 2025

jcouv marked this pull request as ready for review October 21, 2025 23:43

jcouv requested review from a team as code owners October 21, 2025 23:43

jcouv commented Oct 21, 2025

View reviewed changes

build-analysis bot mentioned this pull request Oct 22, 2025

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

Open

3 tasks

AlekseyTs reviewed Oct 22, 2025

View reviewed changes

AlekseyTs previously approved these changes Oct 22, 2025

View reviewed changes

Julien Couvreur added 2 commits October 22, 2025 21:03

Address feedback

35047a9

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

4612df2

jcouv requested a review from AlekseyTs October 23, 2025 04:23

jjonescz approved these changes Oct 23, 2025

View reviewed changes

AlekseyTs reviewed Oct 23, 2025

View reviewed changes

Address feedback

debe12d

AlekseyTs reviewed Oct 23, 2025

View reviewed changes

Add fallback to PickRepresentativeMember

17f9782

jcouv requested a review from AlekseyTs October 23, 2025 21:39

Simplify makeErrorResult parameters

a99dd03

AlekseyTs approved these changes Oct 23, 2025

View reviewed changes

jcouv enabled auto-merge (squash) October 23, 2025 22:25

jcouv mentioned this pull request Oct 23, 2025

Extensions: diagnostic quality follow-ups #78830

Open

5 tasks

jcouv merged commit fa5d81f into dotnet:main Oct 24, 2025
27 of 28 checks passed

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

jcouv deleted the extensions-diagnostics branch October 24, 2025 06:34

This was referenced Oct 26, 2025

[Automated] PRs inserted in VS build main-11125.25 #80900

Closed

[Automated] PRs inserted in VS build feature.debugger.main-11127.165 #80926

Closed

jcouv restored the extensions-diagnostics branch October 28, 2025 07:50

dotnet-maestro bot mentioned this pull request Oct 29, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3147

Open

Uh oh!

Extensions: address some diagnostic quality issues #80827

Extensions: address some diagnostic quality issues #80827

Uh oh!

Conversation

jcouv commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 22, 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 22, 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 22, 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 22, 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 22, 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 22, 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 23, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Oct 23, 2025

Uh oh!

AlekseyTs Oct 23, 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 23, 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 23, 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 23, 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 23, 2025

Uh oh!

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

Uh oh!

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 22, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading

AlekseyTs Oct 23, 2025 •

edited

Loading