We should also test that

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

is not considered circular #Resolved

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.

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

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.

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.

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.

// 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

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

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?