Skip to content

C#: Support for record structs #7605

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

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

michaelnebel
Copy link
Contributor

In this PR we make support for record structs and change the original implementation for record.
Characteristics of the old implementation

  • When records were introduced in C# they were implicitly record types.
  • The QL code used the presence of a method to decide, whether a type should be considered a record type. The method doesn't exist for record struct's, which means we can't reuse this idea to identify, whether a type is a record.

It appears that there is nothing in IL level that identifies, whether something is a record type (it seems native to C# only).
In C# 10, it is now also possible to declare a record struct and record class. record class is clarification of record (that is record is just a shorthand for record class). That is record implicitly means record class and as such, we can consider the record keyword to be a modifier that applies to class and struct.

@github-actions github-actions bot added the C# label Jan 14, 2022
@michaelnebel
Copy link
Contributor Author

@tamasvajk @hvitved This is the initial suggestion for record struct support. I am not sure,

  • if it is acceptable that we remove some of the functionality from cil
  • if we should delete the RecordClone and friends classes.

@tamasvajk
Copy link
Contributor

  • if we should delete the RecordClone and friends classes.

I don't think we should delete RecordClone altogether. That's used as the target of a ... with { ... } expression and might be useful for query writers. BTW, we might need to change the WithExpr in QL to support the C# 10 changes. I added a separate point to the internal C# 10 issue.

@michaelnebel
Copy link
Contributor Author

  • if we should delete the RecordClone and friends classes.

I don't think we should delete RecordClone altogether. That's used as the target of a ... with { ... } expression and might be useful for query writers. BTW, we might need to change the WithExpr in QL to support the C# 10 changes. I added a separate point to the internal C# 10 issue.

Great - thank you! 😄

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Approach looks good to me.

@michaelnebel michaelnebel marked this pull request as ready for review January 19, 2022 09:20
@michaelnebel michaelnebel requested a review from a team as a code owner January 19, 2022 09:20
@michaelnebel michaelnebel requested a review from hvitved January 19, 2022 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants