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

Rekkonnect · 2023-05-21T18:27:19Z

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 · 2023-05-22T13:47:08Z

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

                //         void f() { if () const int i = 0; }
-                Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "f").WithArguments("f").WithLocation(6, 14)
-                );
+                Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "f").WithArguments("f").WithLocation(6, 14));


Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "f").WithArguments("f").WithLocation(6, 14));

Please restore original formatting. #Closed

AlekseyTs · 2023-05-22T13:48:49Z

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

-                );
+                // (6,51): error CS0133: The expression being assigned to 'b' must be constant
+                //         const Func<int> a = () => { const int b = a(); return 1; };
+                Diagnostic(ErrorCode.ERR_NotConstantExpression, "a()").WithArguments("b").WithLocation(6, 51));


);

Please restore original formatting around the placement of the );. This applies to other changes in this file as well. #Closed

AlekseyTs · 2023-05-22T13:50:18Z

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

+            }
+
+            var memberAccessParent = left.Parent as MemberAccessExpressionSyntax;
+            Debug.Assert(memberAccessParent is not null);


Debug.Assert(memberAccessParent is not null);

This assumption looks suspicious and probably is wrong. #Closed

Do you think we can trigger this via a pointer access expression?

AlekseyTs · 2023-05-22T13:52:40Z

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

+            }
+
+            // The first in container binder reflects the current scope, so we begin at the outer scope of the binder
+            for (var binder = firstOuterScopeBinder; binder is not null; binder = binder.Next)


for (var binder = firstOuterScopeBinder; binder is not null; binder = binder.Next)

It is not obvious to me what we are trying to accomplish with this loop. #Closed

We find the first binder that has a broader scope than ours that contains the bound const field. Through observation I figured that we want to find the container type that the binder is associated with, meaning that we expand our lookup into better candidate symbols.

Through observation I figured that we want to find the container type that the binder is associated with, meaning that we expand our lookup into better candidate symbols.

I think, we should not change anything about lookup here, and therefore, shouldn't be changing binders. We also do not need to rebind anything because the meaning of the identifier should not change. It appears that the code does change the meaning and that is wrong. If I understand correctly, the goal is to "circumvent" the ConstantFieldsInProgress tracking. We should be looking for a different way to do that.

AlekseyTs · 2023-05-22T13:54:36Z

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

+            for (var binder = firstOuterScopeBinder; binder is not null; binder = binder.Next)
+            {
+                // While trying to find a suitable binder for the expression, we do not report diagnostics
+                targetBoundValue = binder.BindMemberAccess(memberAccessParent, false, false, BindingDiagnosticBag.Discarded);


targetBoundValue = binder.BindMemberAccess(memberAccessParent, false, false, BindingDiagnosticBag.Discarded);

It is not obvious to me that it is safe to do this while by-passing the constant circularity tracking. #Closed

One case that I could theorize could break this involves nested classes containing visible const fields each. This sure sounds interesting to include as a test case.

AlekseyTs · 2023-05-22T13:56:21Z

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

                                Debug.Assert(!leftType.IsDynamic());
                                Debug.Assert(IsPotentialColorColorReceiver(left, leftType));

+                                if (leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft)


leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft

It is not obvious to me why only these symbols get special treatment. #Closed

I guess that is because ConstantFieldsInProgress tracks only these symbols.

However, it is not obvious to me that all affected scenarios are going to be handled if we apply this condition here.

The only symbols that encounter this bug are fields, and this is the only type that is being tracked by ConstantFieldsInProgress as you said. What kind of affected scenarios are you considering could be missed?

AlekseyTs · 2023-05-22T14:02:31Z

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

+                                {
+                                    boundValue = RebindColorColorConstField(left, boundValue, fieldLeft);
+                                    leftSymbol = boundValue.ExpressionSymbol;
+                                    ConstantFieldsInProgress.RemoveDepencency(fieldLeft);


ConstantFieldsInProgress.RemoveDepencency(fieldLeft);

This looks fragile to me. How do we know that BindIdentifier above was the only reason for the field to be in the set? What if after "rebinding" the reference should still remain in the set? #Closed

This is deeper than I thought; the more the hours were passing, the more convinced I am that we need to approve a spec change, which will make total sense for this line of code.

Essentially, on top of the field initialization, we could entirely exclude the field itself that is being initialized, in order to properly bind to another symbol like a namespace, or type. I will open up a discussion/issue for this matter following further contact.

Essentially, on top of the field initialization, we could entirely exclude the field itself that is being initialized, in order to properly bind to another symbol like a namespace, or type. I will open up a discussion/issue for this matter following further contact.

I think changing the spec is premature. Even if this particular attempt to address the issue isn't something we are comfortable with, it doesn't mean there is no alternative way to approach the problem. We can "sit" on the issue a little longer and let someone from the compiler team to take a closer look once we have time.

AlekseyTs · 2023-05-22T14:07:33Z

Done with review pass (commit 2)

AlekseyTs · 2023-05-22T14:09:01Z

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

AlekseyTs · 2023-05-22T14:27:32Z

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

+
+                // We want to avoid binding to the const field symbol,
+                // so we break on the first symbol that does not equal that
+                if (!boundValue.ExpressionSymbol.Equals(targetBoundValue.ExpressionSymbol))


ExpressionSymbol

ExpressionSymbol is very "imprecise". If we assume that we are dealing with BoundFields and FieldSymbols, then the code should be written in their terms. #Closed

I didn't know you could reliably bound field expressions, perhaps I could try that out

AlekseyTs · 2023-05-22T14:33:25Z

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

+                                if (leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft)
+                                {
+                                    boundValue = RebindColorColorConstField(left, boundValue, fieldLeft);
+                                    leftSymbol = boundValue.ExpressionSymbol;


leftSymbol = boundValue.ExpressionSymbol;

Why is this necessary? Do we expect the symbol to change? #Closed

We do rebind the symbol, and upon returning the bound expression a few lines below this, we also provide the left symbol that is the dependency for this constant's dependency graph, resolving the issue with the circular reference. After having successfully rebound the symbol, we avoid introducing the field itself into the dependency graph as well.

We do rebind the symbol, and upon returning the bound expression a few lines below this, we also provide the left symbol that is the dependency for this constant's dependency graph, resolving the issue with the circular reference. After having successfully rebound the symbol, we avoid introducing the field itself into the dependency graph as well.

This doesn't answer the question: "Do we expect the symbol to change?"

More specifically. Why is this the right thing to replace the field symbol with a symbol that the enclosing member access resolves to?

AlekseyTs · 2023-05-22T14:36:24Z

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

+                }
+            }
+
+            return targetBoundValue;


targetBoundValue

I am not sure what we are doing here. It looks like we are replacing result of binding an identifier with result of binding the enclosing member access. #Closed

This is the exact logic, I thought I described that in the summary of the PR. The point is to rebind to a visible member that is not the constant field itself that is being accessed, because under the current circumstances, you can never yield a constant expression when accessing a constant expression's member.

This is the exact logic, I thought I described that in the summary of the PR

Well, replacing result of binding an identifier with result of binding the enclosing member access is not the right thing to do in my opinion. Whether described or not.

AlekseyTs · 2023-05-22T14:40:48Z

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

+            for (var binder = firstOuterScopeBinder; binder is not null; binder = binder.Next)
+            {
+                // While trying to find a suitable binder for the expression, we do not report diagnostics
+                targetBoundValue = binder.BindMemberAccess(memberAccessParent, false, false, BindingDiagnosticBag.Discarded);


false, false

It is not obvious why false, false are the right values to pass here. #Closed

Because I think we will never fall into the case of rebinding the LHS of a MAE that is a constant field, if the expression is indexed or invoked. But I'm not entirely sure about that either.

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

Rekkonnect · 2023-05-24T20:32:39Z

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 · 2023-05-24T20:47:13Z

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 · 2023-05-24T21:13:31Z

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 · 2023-05-24T21:15:33Z

@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.

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

AlekseyTs · 2023-05-26T14:26:15Z

Done with review pass (commit 6)

AlekseyTs · 2023-05-28T01:33:00Z

@Rekkonnect It looks like there are legitimate test failures

Rekkonnect · 2023-05-28T08:28:05Z

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 · 2023-05-29T16:08:50Z

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 · 2023-05-29T19:55:13Z

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

jaredpar · 2023-07-18T22:03:15Z

Moving to Backlog due to inactivity

Rekkonnect · 2023-07-24T08:34:08Z

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

Rekkonnect · 2024-10-19T15:28:45Z

@AlekseyTs ptal

Rekkonnect · 2025-10-30T09:16:27Z

@AlekseyTs this is ready for review

AlekseyTs · 2025-10-31T14:44:15Z

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

        private readonly SourceFieldSymbol _fieldOpt;
        private readonly HashSet<SourceFieldSymbolWithSyntaxReference> _dependencies;

+        private SourceFieldSymbolWithSyntaxReference _recentDependency;


_recentDependency

It would be good to add e comment explaining the meaning of this field #Closed

Added comment

AlekseyTs · 2025-10-31T14:44:36Z

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

+            _recentDependency = field;
+        }
+
+        internal void RemoveDepencency(SourceFieldSymbolWithSyntaxReference field)


RemoveDepencency

Is this method used? #Closed

Removed as unused after inlining

AlekseyTs · 2025-10-31T14:46:22Z

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

+
+        private bool EqualsRecentDependency(SourceFieldSymbolWithSyntaxReference field)
+        {
+            return _recentDependency == field;


_recentDependency == field

_recentDependency == (object)field? #Closed

Added (object) cast

AlekseyTs · 2025-10-31T14:49:48Z

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

+
+        internal void RemoveIfEqualsRecentDependency(SourceFieldSymbolWithSyntaxReference field)
+        {
+            if (EqualsRecentDependency(field))


EqualsRecentDependency

Consider inlining the logic #Closed

AlekseyTs · 2025-10-31T14:50:02Z

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

+        {
+            if (EqualsRecentDependency(field))
+            {
+                RemoveRecentDependency();


RemoveRecentDependency

Consider inlining the logic #Closed

Rekkonnect · 2025-10-31T19:55:55Z

@AlekseyTs changed the code, ptal

AlekseyTs · 2025-10-31T20:35:17Z

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

-using Roslyn.Utilities;

-namespace Microsoft.CodeAnalysis.CSharp
+namespace Microsoft.CodeAnalysis.CSharp;


Style changes like that are not welcome under Compilers, please revert. #Closed

Reverted to block-scoped

AlekseyTs · 2025-11-01T17:57:16Z

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


+                                if (leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft)
+                                {
+                                    ConstantFieldsInProgress.RemoveIfLastDependency(fieldLeft);


ConstantFieldsInProgress.RemoveIfLastDependency(fieldLeft);

I still find the approach to adjust/fixup the ConstantFieldsInProgress set fragile and bug prone. For example, it doesn't look like current implementation properly handles the following scenario:

public const Color Color = Color | Color.Red;

A cycle should be detected. Instead emit fails with the following error:

error CS7038: Failed to emit module '...': Unable to determine specific cause of the failure.

I prefer an alternative approach that avoids premature binding of the value. It is implemented in #80978.

I like your PR more than this one, it does the job more cleanly. We can close this one out unless the other PR doesn't go anywhere

Let's wait for it to go through code review process

AlekseyTs · 2025-11-01T17:59:52Z

Done with review pass (commit 15)

Remove dependency of ColorColor const assignment

536c413

Rekkonnect requested a review from a team as a code owner May 21, 2023 18:27

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 21, 2023

Remove unused method parameter

b145850

AlekseyTs reviewed May 22, 2023

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ColorColorTests.cs Outdated Show resolved Hide resolved

Rekkonnect added 2 commits May 25, 2023 19:16

Revert changes made to ConstantTests.cs

b896c53

Revert rebinding process preserving const binding

78ab599

Rekkonnect requested a review from AlekseyTs May 25, 2023 16:55

Rekkonnect added 2 commits May 25, 2023 20:07

Improve test cases

db11e2a

Add console output in tests

136e10e

build-analysis bot mentioned this pull request May 25, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

AlekseyTs reviewed May 26, 2023

View reviewed changes

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

Apply suggestions and add extra test

3aa6500

Rekkonnect requested a review from AlekseyTs May 26, 2023 23:41

Remove unwanted NRT annotation

d941af1

jaredpar added this to the Backlog milestone Jul 18, 2023

Rekkonnect added 3 commits July 23, 2023 00:46

Merge branch 'main' into fix-45739

4f216e0

Fix NRE when const fields in progress is empty

c4f3c8f

Update WorkItem attributes

0a36d41

Merge branch 'main' into fix-45739

319d714

Merge branch 'main' into fix-45739

a2ecc0c

AlekseyTs reviewed Oct 31, 2025

View reviewed changes

simplify and doc logic

c27d5b9

AlekseyTs reviewed Oct 31, 2025

View reviewed changes

use block scoped namespace

06cbfea

AlekseyTs reviewed Nov 1, 2025

View reviewed changes

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

Are you sure you want to change the base?

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

Conversation

Rekkonnect commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Example

Uh oh!

AlekseyTs May 22, 2023 • 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 May 22, 2023 • 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 May 22, 2023 • 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 May 22, 2023 • 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 May 22, 2023 • 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 May 22, 2023 • 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!

AlekseyTs May 22, 2023 • 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 commented May 22, 2023

Uh oh!

AlekseyTs commented May 22, 2023

Uh oh!

AlekseyTs May 22, 2023 • 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 May 22, 2023 • 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!

AlekseyTs May 22, 2023 • 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!

Rekkonnect commented May 21, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs May 22, 2023 •

edited

Loading

AlekseyTs commented May 24, 2023 •

edited

Loading

AlekseyTs commented May 29, 2023 •

edited

Loading

AlekseyTs Oct 31, 2025 •

edited

Loading

AlekseyTs Oct 31, 2025 •

edited

Loading