Fix Color Color const field initialization accessing the Color type

[Rekkonnect]

Closes #45739

Summary

Color Color cases of const field initializations would trigger a circular dependency error. This bug was being caused in the constant initializer dependency graph calculation. Another contributor was the binder resolving Color as the field, rather than the type itself.

Details

When binding inside BindLeftIdentifierOfPotentialColorColorMemberAccess in Binder_Expressions.cs, we also remove the dependency of the const field itself from the graph, if it is part of the dependencies, which in turn eliminates the circular dependency error. This came with the introduction of a RemoveIfEqualsRecentDependency method in ConstantFieldsInProgress, to directly remove the dependency from the set that was passed down during the evaluation of the field's constant initialization, if it was the recent dependency in the graph, which is also tracked down upon adding the dependency.

Example

As added in the test, the following simple case determines the existence of the bug:

enum Color
{
    Red,
    Green,
    Blue,
    Yellow,
}

class M
{
    public const Color Color = Color.Red;
}

Color.Red was previously referring to the const field Color, rather than the enum Color. This caused a viable circular dependency error, due to the mistaken binding.

Now, Color.Red is resolved as accessing the Red member of the Color enum type.

[AlekseyTs]

Done with review pass (commit 2)

[AlekseyTs]

@Rekkonnect Just in case, please do not do a forced push to a PR under review.

[Rekkonnect]

As discussed in dotnet/csharplang#7222, this PR will only focus on fixing the Color Color cases of constant field initializers and enum field initializers. Namespace binding is not intended from the spec and will not be covered in this fix.

[AlekseyTs]

Namespace binding is not intended from the spec and will not be covered in this fix.

I am not sure how to interpret this. Also, we are not making compiler changes base on dotnet/csharplang discussions. In my opinion, the language rules are fine and there is no need to change them. There is an implementation flaw in the way constants are handled in Color Color scenarios, there is an issue that tracks fixing the flaw. The approach taken in this PR to address the flaw is wrong, in my opinion. I suggest letting compiler team to tackle the issue, unless you have an alternative in mind.

[Rekkonnect]

I can understand the approach being wrong or clunky, it doesn't completely resonate too sanely in my mind either.

What I meant by not handling namespace binding in this PR is that the spec does not permit binding a Color Color case into a namespace qualifier, only a type qualifier. The linked discussion that was held explicitly shows how namespaces are not meant to be handled. Should the spec change in the future, or have another case appear as erroneous, another PR might be suitable for it.

The purpose of this PR is to fix the bug, according to the current spec. As it stands, const field initializers and enum field initializers should not cause this error, meaning that the Color Color case should be handled in those places. This PR should fix those two cases.

There might be some language barrier in interpreting my previous comment, hope this comment clears this out.

[CyrusNajmabadi]

@AlekseyTs from offline discussion with Rekkonect, there is an understanding that the only 'issue' under consideration here is around the Color/Color case for constants (either constant fields, or enum members). Currently, it seems like the compiler is out of spec with the language (though let me know if you disagree with that). Up to compiler team on if they would want to pursue any changes in the impl to bring it in line with what we think the spec says here.

I think he was simply stating that other things he noticed while looking into this are not actually related to this topic and are out of scope from this actually identified potential issue with constants.

[AlekseyTs]

Done with review pass (commit 6)

[AlekseyTs]

@Rekkonnect It looks like there are legitimate test failures

[Rekkonnect]

This specific test that failed passes just fine in me. Could it be some pooling bug when running all the tests? I only ran a small subset of related tests to make sure the feature passes, but failed to cause the NRE.

[AlekseyTs]

Judging by the call stack, the crash looks primarily related to the changes made in this PR:

Microsoft.CodeAnalysis.CSharp.UnitTests.ColorColorTests.ConstFieldPrimitivesActualCircular

System.NullReferenceException : Object reference not set to an instance of an object.

Stack trace
   at Microsoft.CodeAnalysis.CSharp.ConstantFieldsInProgress.AddDependency(SourceFieldSymbolWithSyntaxReference field) in /_/src/Compilers/CSharp/Portable/Binder/ConstantFieldsInProgress.cs:line 42
   at Microsoft.CodeAnalysis.CSharp.Binder.ReplaceTypeOrValueReceiver(BoundExpression receiver, Boolean useType, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 1591
   at Microsoft.CodeAnalysis.CSharp.Binder.BindInvocationExpressionContinued(SyntaxNode node, SyntaxNode expression, String methodName, OverloadResolutionResult`1 result, AnalyzedArguments analyzedArguments, MethodGroup methodGroup, NamedTypeSymbol delegateTypeOpt, BindingDiagnosticBag diagnostics, CSharpSyntaxNode queryClause) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 1073
   at Microsoft.CodeAnalysis.CSharp.Binder.BindMethodGroupInvocation(SyntaxNode syntax, SyntaxNode expression, String methodName, BoundMethodGroup methodGroup, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics, CSharpSyntaxNode queryClause, Boolean allowUnexpandedForm, Boolean& anyApplicableCandidates) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 741
   at Microsoft.CodeAnalysis.CSharp.Binder.BindInvocationExpression(SyntaxNode node, SyntaxNode expression, String methodName, BoundExpression boundExpression, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics, CSharpSyntaxNode queryClause, Boolean allowUnexpandedForm) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 321
   at Microsoft.CodeAnalysis.CSharp.Binder.<BindInvocationExpression>g__bindArgumentsAndInvocation|564_0(InvocationExpressionSyntax node, BoundExpression boundExpression, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 225
   at Microsoft.CodeAnalysis.CSharp.Binder.BindInvocationExpression(InvocationExpressionSyntax node, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:line 214
   at Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionInternal(ExpressionSyntax node, BindingDiagnosticBag diagnostics, Boolean invoked, Boolean indexed) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:line 574
   at Microsoft.CodeAnalysis.CSharp.Binder.BindExpression(ExpressionSyntax node, BindingDiagnosticBag diagnostics, Boolean invoked, Boolean indexed) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:line 519
   at Microsoft.CodeAnalysis.CSharp.Binder.BindValue(ExpressionSyntax node, BindingDiagnosticBag diagnostics, BindValueKind valueKind) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:line 237
   at Microsoft.CodeAnalysis.CSharp.Binder.BindPossibleArrayInitializer(ExpressionSyntax node, TypeSymbol destinationType, BindValueKind valueKind, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs:line 1821
   at Microsoft.CodeAnalysis.CSharp.Binder.BindVariableOrAutoPropInitializerValue(EqualsValueClauseSyntax initializerOpt, RefKind refKind, TypeSymbol varType, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:line 461
   at Microsoft.CodeAnalysis.CSharp.Binder.BindFieldInitializer(FieldSymbol field, EqualsValueClauseSyntax initializerOpt, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:line 441
   at Microsoft.CodeAnalysis.CSharp.InitializerSemanticModel.BindEqualsValue(Binder binder, EqualsValueClauseSyntax equalsValue, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Compilation/InitializerSemanticModel.cs:line 160
   at Microsoft.CodeAnalysis.CSharp.InitializerSemanticModel.Bind(Binder binder, CSharpSyntaxNode node, BindingDiagnosticBag diagnostics) in /_/src/Compilers/CSharp/Portable/Compilation/InitializerSemanticModel.cs:line 140
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.<EnsureNullabilityAnalysisPerformedIfNecessary>g__bind|130_0(CSharpSyntaxNode root, Binder& binder, <>c__DisplayClass130_0&) in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 1966
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.EnsureNullabilityAnalysisPerformedIfNecessary() in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 1941
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.GetBoundNodes(CSharpSyntaxNode node) in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 2052
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.GetUpperBoundNode(CSharpSyntaxNode node, Boolean promoteToBindable) in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 523
   at Microsoft.CodeAnalysis.CSharp.InitializerSemanticModel.GetBoundRoot() in /_/src/Compilers/CSharp/Portable/Compilation/InitializerSemanticModel.cs:line 108
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.GetRootOperation() in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 1178
   at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.GetOperationWorker(CSharpSyntaxNode node, CancellationToken cancellationToken) in /_/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:line 1157
   at Microsoft.CodeAnalysis.CSharp.SyntaxTreeSemanticModel.GetOperationWorker(CSharpSyntaxNode node, CancellationToken cancellationToken) in /_/src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs:line 190
   at Microsoft.CodeAnalysis.CSharp.CSharpSemanticModel.GetOperationCore(SyntaxNode node, CancellationToken cancellationToken) in /_/src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs:line 479
   at Microsoft.CodeAnalysis.SemanticModel.GetOperation(SyntaxNode node, CancellationToken cancellationToken) in /_/src/Compilers/Core/Portable/Compilation/SemanticModel.cs:line 78
   at Microsoft.CodeAnalysis.Test.Utilities.CompilationExtensions.ValidateIOperations(Func`1 createCompilation) in /_/src/Compilers/Test/Core/Compilation/CompilationExtensions.cs:line 299
   at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.ValidateCompilation(Func`1 createCompilationLambda) in /_/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs:line 1263
   at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CreateCompilationCore(CSharpTestSource source, IEnumerable`1 references, CSharpCompilationOptions options, CSharpParseOptions parseOptions, String assemblyName, String sourceFileName, Boolean skipUsesIsNullable, Nullable`1 experimentalFeature, Boolean skipExtraValidation) in /_/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs:line 1239
   at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CreateEmptyCompilation(CSharpTestSource source, IEnumerable`1 references, CSharpCompilationOptions options, CSharpParseOptions parseOptions, String assemblyName, String sourceFileName, Boolean skipUsesIsNullable, Boolean skipExtraValidation) in /_/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs:line 1203
   at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CreateCompilation(CSharpTestSource source, IEnumerable`1 references, CSharpCompilationOptions options, CSharpParseOptions parseOptions, TargetFramework targetFramework, String assemblyName, String sourceFileName, Boolean skipUsesIsNullable) in /_/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs:line 1192
   at Microsoft.CodeAnalysis.CSharp.UnitTests.ColorColorTests.ConstFieldPrimitivesActualCircular() in /_/src/Compilers/CSharp/Test/Semantic/Semantics/ColorColorTests.cs:line 2454
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
Showing filters 1 through 5

It looks like AddDependency is called on an ConstantFieldsInProgress Empty instance.

The "Test_Windows_CoreClr_IOperation_Debug" CI leg executes extra validation for test scenarios. You should be able to run the test locally in the same mode if you temporarily comment out an if statement at the beginning of

        public static void ValidateIOperations(Func<Compilation> createCompilation)
        {
            if (!EnableVerifyIOperation)
            {
                return;
            }

in src/Compilers/Test/Core/Compilation/CompilationExtensions.cs. #Closed

[Rekkonnect]

Thank you for your elaborate guidance, I will have a look when I have the time and have it fixed for that scenario

[jaredpar]

Moving to Backlog due to inactivity

[Rekkonnect]

@AlekseyTs the test case was fixed and all checks pass now, I think the PR is ready for a final review

[Rekkonnect]

@AlekseyTs ptal

[Rekkonnect]

@AlekseyTs this is ready for review

[Rekkonnect]

@AlekseyTs changed the code, ptal

[AlekseyTs]

Done with review pass (commit 15)

[Rekkonnect]

Closing as issue closed by #80978