Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GenAPI cannot decode records when reading from metadata #32196

Closed
ericstj opened this issue May 2, 2023 · 5 comments · Fixed by #34433
Closed

GenAPI cannot decode records when reading from metadata #32196

ericstj opened this issue May 2, 2023 · 5 comments · Fixed by #34433
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented May 2, 2023

Describe the bug

GenAPI does not emit records correctly.

To Reproduce

namepsace ns;
public record class Foo();

Compile this and point GenAPI to it.

Expect: Same output

Actual:

namespace ns
{
    public record Foo
    {
        public Foo() { }
        protected Foo(Foo original) { }
        protected virtual System.Type EqualityContract { get { throw null; } }
        public virtual Foo <Clone>$() { throw null; }
        public virtual System.Boolean? Equals(Foo? other) { throw null; }
        public override System.Boolean? Equals(System.Object? obj) { throw null; }
        public override System.Int32 GetHashCode() { throw null; }
        public static System.Boolean? operator ==(Foo? left, Foo? right) { throw null; }
        public static System.Boolean? operator !=(Foo? left, Foo? right) { throw null; }
        protected virtual System.Boolean PrintMembers(System.Text.StringBuilder builder) { throw null; }
        public override System.String ToString() { throw null; }
    }
}

Related: #32165

AFAICT there isn't anything preserved in metadata to indicate that a type is a record. Perhaps we could use a heuristic -- look for all the members https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/records.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label May 2, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ViktorHofer
Copy link
Member

Related: dotnet/roslyn#67335

@KalleOlaviNiemitalo
Copy link
Contributor

Perhaps we could use a heuristic -- look for all the members

record struct synthesizes fewer members. https://learn.microsoft.com/dotnet/csharp/language-reference/proposals/csharp-10.0/record-structs

@ViktorHofer
Copy link
Member

This seems to be important, especially when 8.0 packages will be added to SBRP. Keeping in 8.0 for now.

@ViktorHofer ViktorHofer added this to the 8.0.1xx milestone Jul 12, 2023
@ViktorHofer ViktorHofer removed the untriaged Request triage from a team member label Jul 12, 2023
@ericstj
Copy link
Member Author

ericstj commented Aug 2, 2023

Seems like I was wrong in my statement above - genapi does recognize the type as a record type - when it happens to be from source, or a record class. It misses record structs when read from metadata - I filed dotnet/roslyn#69326. Interestingly - for a record struct ignoring that it's a record doesn't make a difference -- it's just more verbose.

After realizing this I thought that this bug might just be cosmetic - that we'd emit more than necessary but it would still compile. This is not the case for record classes because there are members included that cannot be represented in C# (eg: <Clone>$()).

Perhaps we can just filter out all compiler generated members on record classes. I gave this a try manually and found that it wasn't quite enough - since it looks like the compiler doesn't annotate the public constructor with CompilerGenerated and defining it results in the compiler omitting the Deconstruct method (which does have the annotation). We could omit the generated constructor - we'd have to identify it as the one constructor that doesn't call any others (this(..)).

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

Successfully merging a pull request may close this issue.

3 participants