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

Fix inconsistencies for record types across NET 6.x.x SDK #247

Merged
merged 12 commits into from Dec 30, 2022
Merged

Fix inconsistencies for record types across NET 6.x.x SDK #247

merged 12 commits into from Dec 30, 2022

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Aug 11, 2022

fixes #245

@sungam3r
Copy link
Member Author

@Shane32 3.1 runtime fails all tests, 6.0.400 success on both Windows and Linux!

@sungam3r
Copy link
Member Author

It seems that there are some inconsistencies between CompilerGenerated attributes accross SDKs. Nevermind, I just ignored EqualityContract member to unify behavior. Also NET5 (and even NET 6.0.100) has completely different output from NET6:

 public class User : System.IEquatable<PublicApiGeneratorTests.Examples.User>
    {
        protected User(PublicApiGeneratorTests.Examples.User original) { }
        public User(string login, string password) { }
        public string login { get; set; }
        public string password { get; set; }
        public virtual PublicApiGeneratorTests.Examples.User <Clone>$() { }
        public void Deconstruct(out string login, out string password) { }
        public virtual bool Equals(PublicApiGeneratorTests.Examples.User? other) { }
        public override bool Equals(object? obj) { }
        public override int GetHashCode() { }
        protected virtual bool PrintMembers(System.Text.StringBuilder builder) { }
        public override string ToString() { }
        public static bool operator !=(PublicApiGeneratorTests.Examples.User? left, PublicApiGeneratorTests.Examples.User? right) { }
        public static bool operator ==(PublicApiGeneratorTests.Examples.User? left, PublicApiGeneratorTests.Examples.User? right) { }

vs

public class User : System.IEquatable<PublicApiGeneratorTests.Examples.User>
    {
        public User(string login, string password) { }
        public string login { get; set; }
        public string password { get; set; }
    }

@sungam3r sungam3r changed the title Add tests for records Fix inconsistencies for record types across SDKs Aug 11, 2022
@sungam3r sungam3r changed the title Fix inconsistencies for record types across SDKs Fix inconsistencies for record types across NET 6.x.x SDK Aug 11, 2022
@Shane32
Copy link
Contributor

Shane32 commented Aug 11, 2022

I think I remember a .NET compiler bug saying that the generated members were not marked as compiler generated or something. I don't have a reference, but chances are the bug was resolved after 6.0.100 and hence the difference.

@sungam3r
Copy link
Member Author

@stakx I will be grateful to you for review/publish the fix.

@sungam3r sungam3r mentioned this pull request Aug 14, 2022
@stakx
Copy link
Collaborator

stakx commented Aug 15, 2022

@sungam3r, I'm about to go on vacation for a few weeks' time. If time permits, I'll try to review this before I'm off.

@Shane32
Copy link
Contributor

Shane32 commented Aug 15, 2022

@stakx It would be greatly appreciated

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Co-authored-by: Shane Krueger <shane@acdmail.com>
@sungam3r
Copy link
Member Author

Also affects EasyNetQ/EasyNetQ#1441

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Co-authored-by: Shane Krueger <shane@acdmail.com>
Copy link
Collaborator

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a few comments on some minor points.

.github/workflows/ci.yml Show resolved Hide resolved
src/PublicApiGeneratorTests/Assembly_attributes.cs Outdated Show resolved Hide resolved
@stakx
Copy link
Collaborator

stakx commented Aug 27, 2022

Also NET5 (and even NET 6.0.100) has completely different output from NET6

Like @Shane32 I also suspect this may be related to [CompilerGenerated] attribute now emitted. .NET 6 emits record types differently than .NET 5 did (e.g. it emits clone methods using covariant returns), that may be one of those differences. There's likely nothing that can be done about the "superfluous" members appearing for .NET 5.

@sungam3r
Copy link
Member Author

There's likely nothing that can be done about the "superfluous" members appearing for .NET 5.

Yes. Let's consider them as a "normal" parts of public api set.

@sungam3r
Copy link
Member Author

Sorry for delay.

@sungam3r
Copy link
Member Author

sungam3r commented Oct 5, 2022

@stakx OK to merge?

@sungam3r
Copy link
Member Author

sungam3r commented Nov 3, 2022

@stakx ?

@danielmarbach
Copy link
Member

@sungam3r I could merge this since I still have contributor rights but I can't release it. Let me know

@sungam3r
Copy link
Member Author

As you wish. We have already upgraded our CI to 6.0.402 and 7.0.101.

@stakx stakx merged commit c88cd32 into PublicApiGenerator:master Dec 30, 2022
@stakx
Copy link
Collaborator

stakx commented Dec 30, 2022

@sungam3r, my apologies for letting this PR sit around for such a long time. I've lately been unexpectedly busy at work and needed some time off from programming-related activities in my spare time. I'm now finally going through all the PRs that have accumulated since.

Sorry for that again!

@sungam3r
Copy link
Member Author

busy at work and needed some time off from programming-related activities in my spare time

The same thing.

@sungam3r sungam3r deleted the record branch December 30, 2022 22:04
@sungam3r
Copy link
Member Author

@stakx To keep history clean I suggest to use squash option
изображение

Current history with merge-commits:
изображение

And also this
изображение

@stakx
Copy link
Collaborator

stakx commented Dec 30, 2022

@sungam3r, I agree that squashing these commits would have resulted in a cleaner commit history, and I was almost going to squash them... but then decided against it at the last minute because I didn't want to risk erasing the commit (co-) authors.

@sungam3r
Copy link
Member Author

I didn't want to risk erasing the commit (co-) authors

This info is preserved. Before merge you can edit final commit message. Usually I just CTRL+A and DELETE all that stuff. As a rule only PR title matters and flows into final commit message... but again - you can change it each time as you want.

@stakx
Copy link
Collaborator

stakx commented Dec 30, 2022

I knew that I could edit the final commit message, but I wasn't sure about the co-authorships, which cannot be edited through GitHub's webpage. Good to know it's preserved... thanks for letting me know! 👍

@sungam3r
Copy link
Member Author

Nevertheless, it's better to check twice :).

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

Successfully merging this pull request may close these issues.

API string generated differently on Linux vs Windows under 6.0.400
4 participants