Skip to content

Exclude constant and static fields from record's PrintMembers#47868

Merged
jcouv merged 4 commits intodotnet:masterfrom
Youssef1313:patch-14
Sep 20, 2020
Merged

Exclude constant and static fields from record's PrintMembers#47868
jcouv merged 4 commits intodotnet:masterfrom
Youssef1313:patch-14

Conversation

@Youssef1313
Copy link
Member

Might fix #47867, will confirm after adding a test.

But:

  1. Should constants be excluded?
  2. Why constants currently cause a problem in the first place?

@Youssef1313 Youssef1313 requested a review from a team as a code owner September 19, 2020 18:16
if (m.Kind is SymbolKind.Field)
if (m is SymbolField { IsConst: false })
{
return true;
Copy link
Contributor

@cston cston Sep 19, 2020

Choose a reason for hiding this comment

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

true [](start = 27, length = 4)

We should exclude static fields and properties as well. See sharplab.io.

Copy link
Member Author

@Youssef1313 Youssef1313 Sep 19, 2020

Choose a reason for hiding this comment

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

I have added a check for IsStatic. I think IsConst should be redundant now because constant fields are implicitly static?

I'm also very interested to know why constants are causing issues given that the generated code looks okay.

@Youssef1313 Youssef1313 changed the title Exclude constant fields from record's PrintMembers Exclude constant and static fields from record's PrintMembers Sep 19, 2020
@jcouv
Copy link
Member

jcouv commented Sep 19, 2020

Thanks for jumping on this.
Here are the tests I wanted to add. I'll let you incorporate:


        [Fact]
        public void ToString_RecordWithStaticMembers()
        {
            var src = @"
var c1 = new C1(42);
System.Console.Write(c1.ToString());

record C1(int I1)
{
    public static int field1 = 44;
    public const int field2 = 44;
    public static int P1 { set { } }
    public static int P2 { get { return 1; } set { } }
    public static int P3 { get { return 1; } }
}
";

            var comp = CreateCompilation(new[] { src, IsExternalInitTypeDefinition }, options: TestOptions.DebugExe);
            CompileAndVerify(comp, expectedOutput: "C1 { I1 = 42 }", verify: Verification.Skipped /* init-only */);
            comp.VerifyEmitDiagnostics();
        }

        [Fact]
        public void ToString_NestedRecord()
        {
            var src = @"
var c1 = new Outer.C1(42);
System.Console.Write(c1.ToString());

public class Outer
{
    public record C1(int I1);
}
";

            var comp = CreateCompilation(new[] { src, IsExternalInitTypeDefinition }, options: TestOptions.DebugExe);
            CompileAndVerify(comp, expectedOutput: "C1 { I1 = 42 }", verify: Verification.Skipped /* init-only */);
            comp.VerifyEmitDiagnostics();
        }

@jcouv jcouv self-assigned this Sep 19, 2020
@Youssef1313
Copy link
Member Author

@cston Thanks for writing these tests!
I've added them with only a minor change (testing debug+release).
I'm not sure if this is valuable or not, but I done that because currently, there is a different behavior between Debug and Release.

See #47867 (comment)

@Youssef1313 Youssef1313 marked this pull request as draft September 19, 2020 19:08
@Youssef1313 Youssef1313 marked this pull request as ready for review September 19, 2020 19:08
@jcouv
Copy link
Member

jcouv commented Sep 19, 2020

Corresponding spec update: dotnet/csharplang#3919

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 4)

@jcouv jcouv merged commit 585e569 into dotnet:master Sep 20, 2020
@ghost ghost added this to the Next milestone Sep 20, 2020
@jcouv
Copy link
Member

jcouv commented Sep 20, 2020

Merged/squashed. Thanks @Youssef1313 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MissingFieldException on record types with consts

4 participants