Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7547,6 +7547,11 @@ private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(Iden
Debug.Assert(!leftType.IsDynamic());
Debug.Assert(IsPotentialColorColorReceiver(left, leftType));

if (leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft)
Copy link
Contributor

@AlekseyTs AlekseyTs May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftSymbol is SourceFieldSymbolWithSyntaxReference fieldLeft

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that is because ConstantFieldsInProgress tracks only these symbols.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
ConstantFieldsInProgress.RemoveIfLastDependency(fieldLeft);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// NOTE: ReplaceTypeOrValueReceiver will call CheckValue explicitly.
boundValue = BindToNaturalType(boundValue, valueDiagnostics);
return new BoundTypeOrValueExpression(left,
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1989,6 +1989,16 @@ private BoundExpression ReplaceTypeOrValueReceiver(BoundExpression receiver, boo
}
else
{
// If we resort to rebinding back to the field, we attempt to add the dependency back
if (typeOrValue.Data.ValueExpression.ExpressionSymbol is SourceFieldSymbolWithSyntaxReference sourceField
&& sourceField.IsConst)
{
if (!ConstantFieldsInProgress.IsEmpty)
{
ConstantFieldsInProgress.AddDependency(sourceField);
}
}

diagnostics.AddRange(typeOrValue.Data.ValueDiagnostics);
return CheckValue(typeOrValue.Data.ValueExpression, BindValueKind.RValue, diagnostics);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

using System.Collections.Generic;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand All @@ -20,6 +19,13 @@ internal sealed class ConstantFieldsInProgress
private readonly SourceFieldSymbol _fieldOpt;
private readonly HashSet<SourceFieldSymbolWithSyntaxReference> _dependencies;

/// <summary>
/// Stores the last dependency that was added to the set. This is used to check if
/// the dependency that was added is not after all a dependency after successful rebinding
/// in Color Color resolution, i.e. `public const Color Color = Color.Red;`
/// </summary>
private SourceFieldSymbolWithSyntaxReference _lastDependency;

internal static readonly ConstantFieldsInProgress Empty = new ConstantFieldsInProgress(null, null);

internal ConstantFieldsInProgress(
Expand All @@ -38,6 +44,16 @@ public bool IsEmpty
internal void AddDependency(SourceFieldSymbolWithSyntaxReference field)
{
_dependencies.Add(field);
_lastDependency = field;
}

internal void RemoveIfLastDependency(SourceFieldSymbolWithSyntaxReference field)
{
if (_lastDependency is not null && _lastDependency == (object)field)
{
_dependencies.Remove(_lastDependency);
_lastDependency = null;
}
}
}
}
102 changes: 102 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/ColorColorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2368,6 +2368,108 @@ instance void .ctor () cil managed
compilation.VerifyDiagnostics();
}

[WorkItem("https://github.com/dotnet/roslyn/issues/45739")]
[Fact]
public void ConstFieldEnum()
{
const string source = @"
using Color2 = Color;

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

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

class Program
{
static void Main()
{
System.Console.Write(M.Color == Color.Red);
System.Console.Write(M.Color2 == Color.Yellow);
}
}
";

CompileAndVerify(source, expectedOutput: "TrueTrue")
.VerifyDiagnostics();
}

[WorkItem("https://github.com/dotnet/roslyn/issues/45739")]
[Fact]
public void ConstFieldPrimitives()
{
const string source = @"
using System;
using SystemChar = System.Char;

class M
{
public const Int32 Int32 = Int32.MaxValue;
public const SystemChar SystemChar = SystemChar.MaxValue;
}

class Program
{
static void Main()
{
System.Console.Write(M.Int32 == Int32.MaxValue);
System.Console.Write(M.SystemChar == SystemChar.MaxValue);
}
}
";

CompileAndVerify(source, expectedOutput: "TrueTrue")
.VerifyDiagnostics();
}

[WorkItem("https://github.com/dotnet/roslyn/issues/45739")]
[Fact]
public void ConstFieldPrimitivesActualCircular()
{
string source = @"
using System;
using SystemChar = System.Char;

class M
{
public const Int32 Int32 = Int32.Increment();
public const SystemChar SystemChar = SystemChar.Increment();
}

public static class Extensions
{
public static int Increment(this int i) => i + 1;
public static char Increment(this ref char c) => ++c;
}
";

var compilation = CreateCompilation(source);

compilation.VerifyDiagnostics(
// (7,24): error CS0110: The evaluation of the constant value for 'M.Int32' involves a circular definition
// public const Int32 Int32 = Int32.Increment();
Diagnostic(ErrorCode.ERR_CircConstValue, "Int32").WithArguments("M.Int32").WithLocation(7, 24),
// (7,32): error CS0133: The expression being assigned to 'M.Int32' must be constant
// public const Int32 Int32 = Int32.Increment();
Diagnostic(ErrorCode.ERR_NotConstantExpression, "Int32.Increment()").WithArguments("M.Int32").WithLocation(7, 32),
// (8,29): error CS0110: The evaluation of the constant value for 'M.SystemChar' involves a circular definition
// public const SystemChar SystemChar = SystemChar.Increment();
Diagnostic(ErrorCode.ERR_CircConstValue, "SystemChar").WithArguments("M.SystemChar").WithLocation(8, 29),
// (8,42): error CS1510: A ref or out value must be an assignable variable
// public const SystemChar SystemChar = SystemChar.Increment();
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "SystemChar").WithLocation(8, 42)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71039")]
public void ConstInAttributes_NoCycle_01()
{
Expand Down