Avoid premature full binding of a receiver as a value in the process of binding possible type-or-value receiver (so-called “Color Color” scenario) #80978

AlekseyTs · 2025-11-01T16:04:08Z

…of binding possible type-or-value receiver (so-called “Color Color” scenario) Fixes dotnet#45739.

Rekkonnect · 2025-11-01T18:04:16Z

src/Compilers/CSharp/Test/Semantic/Semantics/ColorColorTests.cs

+
+        [WorkItem("https://github.com/dotnet/roslyn/issues/45739")]
+        [Fact]
+        public void ConstFieldEnum_02()


We should also test that

public const Color Color = Color.Red | Color.Red;

is not considered circular #Resolved

AlekseyTs · 2025-11-03T15:59:58Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-04T12:11:48Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-05T13:23:53Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-06T14:04:05Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-07T13:44:55Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-10T17:15:56Z

@dotnet/roslyn-compiler Please review #Closed

RikkiGibson · 2025-11-10T20:07:44Z

Taking a look shortly. #Closed

RikkiGibson · 2025-11-10T22:28:58Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Binders/PlaceholderLocalBinder.cs

-                    throw ExceptionUtilities.UnexpectedValue(valueText);
-                }
-                var local = new ObjectAddressLocalSymbol(_containingMethod, name, this.Compilation.GetSpecialType(SpecialType.System_Object), address);
+                var local = new ObjectAddressLocalSymbol(_containingMethod, name, this.Compilation.GetSpecialType(SpecialType.System_Object));


I didn't follow why this change is being made, could you please explain? #Resolved

I didn't follow why this change is being made, could you please explain?

Added asserts exposed a bug in comparison of ObjectAddressLocalSymbols. We are not reusing them and were never considered them equal, i.e. representing the same entity. I went ahead and implemented equality for ObjectAddressLocalSymbol. In the process, I incorporated the code above into constructor, that makes it clear what do we need to compare.

AlekseyTs · 2025-11-11T14:15:42Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2025-11-11T19:42:32Z

@dotnet/roslyn-compiler For a second review #Closed

AlekseyTs · 2025-11-12T14:09:30Z

@dotnet/roslyn-compiler For a second review #Closed

AlekseyTs · 2025-11-12T23:08:03Z

@dotnet/roslyn-compiler For a second review #Closed

AlekseyTs · 2025-11-13T14:01:49Z

@dotnet/roslyn-compiler For a second review #Closed

AlekseyTs · 2025-11-14T14:17:21Z

@dotnet/roslyn-compiler For a second review on a PR opened for review on Nov 1st. #Closed

AlekseyTs · 2025-11-17T15:03:53Z

@dotnet/roslyn-compiler For a second review on a PR opened for review on Nov 1st. #Closed

AlekseyTs · 2025-11-18T14:37:39Z

@dotnet/roslyn-compiler For a second review on a PR opened for review on Nov 1st. #Closed

jcouv · 2025-11-19T06:37:21Z

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs

    {
        public override ConstantValue? ConstantValueOpt => Data?.ConstantValue;

-        public override Symbol? ExpressionSymbol => this.BinaryOperatorMethod;


Regarding comment on BoundTypeOrValueData type (line 669 to 672)

ValueExpression and TypeExpression were removed so the comment above should not reference those members.

Also, the justification provided for this type was to hide some bound nodes from the regular HasErrors bound node handling. Is this still the case?
Could this information be inlined into BoundTypeOrValueExpression? Or should the comment be updated? #Closed

Removed the BoundTypeOrValueData type

jcouv · 2025-11-19T06:41:29Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

                        {
-                            Debug.Assert(symbol.IsExtensionBlockMember());
+                            Debug.Assert(symbol.GetIsNewExtensionMember());
                            if (SourceNamedTypeSymbol.ReduceExtensionMember(binder.Compilation, symbol, receiverType, wasExtensionFullyInferred: out _) is { } compatibleSubstitutedMember)


Regarding comment on case BoundKind.TypeOrValueExpression: in GetSemanticSymbols (line 3424)

Comment above seems out-of-date (remove?) #Closed

Comment above seems out-of-date (remove?)

Adjusted

nit: There's still a problem with the comment. It says:

// If we're seeing a node of this kind, then we failed to resolve the member access // as either a type or a property/field/event/local/parameter. In such cases, // the second interpretation applies.

But then we assert: Debug.Assert(boundNode is not BoundTypeOrValueExpression);

I'd just remove the comment.

The comment is correct. The assert is there to help us to make sure that we do not leave BoundTypeOrValueExpression in the tree. If it is there, we can recover, but we prefer not getting in a situation like that.

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs

jcouv · 2025-11-19T07:08:05Z

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

-                        return CheckValue(typeOrValue.Data.ValueExpression, BindValueKind.RValue, diagnostics);
+                        var boundValue = typeOrValue.Data.Binder.BindIdentifier(identifier, invoked: false, indexed: false, diagnostics: diagnostics);
+
+                        Debug.Assert(typeOrValue.Type.Equals(boundValue.Type, TypeCompareKind.ConsiderEverything));


Debug.Assert(typeOrValue.Type.Equals(boundValue.Type, TypeCompareKind.ConsiderEverything)); (line 1973)
Could the nullability of the value differ? For example Color? Color = ...; the Color value would have type Color? #Closed

Could the nullability of the value differ?

Bound tree doesn't preserve top level nullability of types and we are binding simple identifiers. Perhaps nested nullability can somehow sneak in. Since this is an assert, I am not worried about this and I am comfortable going with a possibly too strong condition. We can relax it once the assert starts causing problems.

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

jcouv · 2025-11-19T07:29:26Z

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

                resultKind,
                symbols,
-                childNodes.SelectAsArray((e, self) => self.BindToTypeForErrorRecovery(e), this),
+                childNodes.SelectAsArray((e, self) => self.AdjustBadExpressionChild(self.BindToTypeForErrorRecovery(e)), this),


self.AdjustBadExpressionChild(self.BindToTypeForErrorRecovery(e)) (line 195)
nit: Would it be simpler for BindToTypeForErrorRecovery to handle calling AdjustBadExpressionChild, even if it involves a tiny bit of extra work in scenarios were a type-or-value couldn't not be encountered? #Resolved

What was the resolution?

What was the resolution?

Since this was a "nit" I didn't spend much time thinking about that and didn't feel like this comment needs a specific response. I took it under advisement. I am comfortable with the current approach.
I felt that suggested simplification won't be appropriate because the node has type, and therefore, should not be changed by a helper the sole purpose of which is to figure out the type, even when that operation is requested as part of an error recovery.

jcouv · 2025-11-19T07:35:03Z

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

                            }
                        }

+                        // The type calculation here should be kept in sync with logic in BindLeftIdentifierOfPotentialColorColorMemberAccess.


// The type calculation here should be kept in sync with logic in BindLeftIdentifierOfPotentialColorColorMemberAccess. (line 2148)
It's not obvious what type calculation this comment is referring to
#Closed

It's not obvious what type calculation this comment is referring to

Clarified

jcouv · 2025-11-19T07:40:38Z

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

                    }

                case SymbolKind.RangeVariable:
+                    // The type calculation here should be kept in sync with logic in BindLeftIdentifierOfPotentialColorColorMemberAccess.


Comment refers to case SymbolKind.Field: a few lines above (line 2171)
~~Should a similar comment be left in the field case?~~
~~BindLeftIdentifierOfPotentialColorColorMemberAccess has a case for fields when computing type~~

Never mind. I see it's in BindFieldAccess #Closed

jcouv · 2025-11-19T07:54:32Z

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

+
+            var lookupResult = LookupResult.GetInstance();
+            var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
+            this.LookupIdentifier(lookupResult, left, invoked: false, ref useSiteInfo);


Not directly related to this PR: Does BindLeftIdentifierOfPotentialColorColorMemberAccess need to worry about nint and _ like BindIdentifier does?
nint nint = 0; and _ _ = null; class _ { } appear legal. #Closed

Does BindLeftIdentifierOfPotentialColorColorMemberAccess need to worry about nint and _ like BindIdentifier does?

Could you please elaborate? What code in BindIdentifier do you refer to?

It looks like this is legal

_ _ = null; _.ReferenceEquals(null, null); class _ { }

This seems correct because - in this case is identifier and the name of the type.

The following is illegal because the name of the type is not "nint", it is "IntPtr"

nint nint = 0; nint.ReferenceEquals(null, null);

However, this works:

using System; nint IntPtr = 0; IntPtr.ReferenceEquals(null, null);

Does this answer the question?

jcouv

Done with review pass (commit 3)

jcouv

Done with review pass (commit 5)

jcouv

LGTM Thanks (commit 5)

Avoid premature full binding of a receiver as a value in the process …

716e35f

…of binding possible type-or-value receiver (so-called “Color Color” scenario) Fixes dotnet#45739.

AlekseyTs requested a review from a team as a code owner November 1, 2025 16:04

AlekseyTs added the Area-Compilers label Nov 1, 2025

Implement Equals/GetHashCode for ObjectAddressLocalSymbol

5dc8330

AlekseyTs mentioned this pull request Nov 1, 2025

Fix Color Color const field initialization accessing the Color type #68283

Closed

Rekkonnect reviewed Nov 1, 2025

View reviewed changes

Fixup

cc03680

RikkiGibson reviewed Nov 10, 2025

View reviewed changes

AlekseyTs requested review from a team and RikkiGibson November 11, 2025 18:36

RikkiGibson approved these changes Nov 11, 2025

View reviewed changes

AlekseyTs requested a review from a team November 11, 2025 19:42

jcouv self-assigned this Nov 19, 2025

jcouv reviewed Nov 19, 2025

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs Outdated Show resolved Hide resolved

jcouv reviewed Nov 19, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs Show resolved Hide resolved

jcouv reviewed Nov 19, 2025

View reviewed changes

AlekseyTs added 2 commits November 19, 2025 23:16

Merge remote-tracking branch 'upstream/main' into Issue45739_2

74d5976

PR feedback

c667931

AlekseyTs requested a review from jcouv November 20, 2025 08:07

jcouv reviewed Nov 20, 2025

View reviewed changes

jcouv approved these changes Nov 20, 2025

View reviewed changes

Clarify the purpose of asserts

9960888

AlekseyTs enabled auto-merge (squash) November 20, 2025 17:10

AlekseyTs merged commit ac2327f into dotnet:main Nov 20, 2025
28 of 29 checks passed

dotnet-policy-service bot added this to the Next milestone Nov 20, 2025

dotnet-maestro bot mentioned this pull request Nov 21, 2025

[release/10.0.2xx] Source code updates from dotnet/roslyn dotnet/dotnet#3484

Open

Avoid premature full binding of a receiver as a value in the process of binding possible type-or-value receiver (so-called “Color Color” scenario) #80978

Avoid premature full binding of a receiver as a value in the process of binding possible type-or-value receiver (so-called “Color Color” scenario) #80978

Conversation

AlekseyTs commented Nov 1, 2025

Uh oh!

Rekkonnect Nov 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

RikkiGibson commented Nov 10, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson Nov 10, 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv Nov 19, 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!

jcouv Nov 19, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Rekkonnect Nov 1, 2025 •

edited by AlekseyTs

Loading

AlekseyTs commented Nov 3, 2025 •

edited

Loading

AlekseyTs commented Nov 4, 2025 •

edited

Loading

AlekseyTs commented Nov 5, 2025 •

edited

Loading

AlekseyTs commented Nov 6, 2025 •

edited

Loading

AlekseyTs commented Nov 7, 2025 •

edited

Loading

AlekseyTs commented Nov 10, 2025 •

edited

Loading

RikkiGibson commented Nov 10, 2025 •

edited by AlekseyTs

Loading

RikkiGibson Nov 10, 2025 •

edited by AlekseyTs

Loading

AlekseyTs commented Nov 11, 2025 •

edited

Loading

AlekseyTs commented Nov 11, 2025 •

edited

Loading

AlekseyTs commented Nov 12, 2025 •

edited

Loading

AlekseyTs commented Nov 12, 2025 •

edited

Loading

AlekseyTs commented Nov 13, 2025 •

edited

Loading

AlekseyTs commented Nov 14, 2025 •

edited

Loading

AlekseyTs commented Nov 17, 2025 •

edited

Loading

AlekseyTs commented Nov 18, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited

Loading

jcouv Nov 19, 2025 •

edited

Loading