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

Avoid caching compilation data and use value equality for SyntaxNodes #79051

Merged
merged 7 commits into from
Jan 3, 2023

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Nov 30, 2022

Fixes #78242.

Creates separate types for InteropAttributeData: one that holds compilation data (InteropAttributeCompilationData), and one that holds only the data necessary for the model to create the generated code (InteropAttributeModelData).

This uncovered some issues with record equality in records that use SyntaxNode. For those, we need to override Equals or wrap the SyntaxNode in a type that overrides Equals to use IsEquivalentTo on the SyntaxNode.

There are probably more places where we use SyntaxNode that aren't caught in the current tests.

To make sure every record has the right equality, I wasn't sure if it would be better to override Equals for each of the records, or create a wrapper record struct for each SyntaxNode that implements the equality we want (and implicit casts to and from the SyntaxNode). Then we wouldn't have to explicitly override the equality in each record that has a SyntaxNode. I also overrode both Equals and GetHashCode, but I'm not confident in my GetHashCode implementation. It could also be done with IEquatable.Equals without needing GetHashCode, but that would require implementing the TypeSyntax equality for every type that inherits from ManagedTypeInfo.

@ghost
Copy link

ghost commented Nov 30, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78242.

Creates separate types for InteropAttributeData: one that holds compilation data (InteropAttributeCompilationData), and one that holds only the data necessary for the model to create the generated code (InteropAttributeModelData).

This uncovered some issues with record equality in records that use SyntaxNode. For those, we need to override Equals or wrap the SyntaxNode in a type that overrides Equals to use IsEquivalentTo on the SyntaxNode.

There are probably more places where we use SyntaxNode that aren't caught in the current tests.

To make sure every record has the right equality, I wasn't sure if it would be better to override Equals for each of the records, or create a wrapper record struct for each SyntaxNode that implements the equality we want (and implicit casts to and from the SyntaxNode). Then we wouldn't have to explicitly override the equality in each record that has a SyntaxNode.

Author: jtschuster
Assignees: jtschuster
Labels:

area-System.Runtime.InteropServices

Milestone: -

@jtschuster jtschuster marked this pull request as ready for review November 30, 2022 18:25
@jtschuster jtschuster added the source-generator Indicates an issue with a source generator feature label Nov 30, 2022
- Add type doc comments for new types
- Don't use SyntaxNode in GetHashCode implementation
- Add comment on Dictionary equality check
Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

This uncovered some issues with record equality in records that use SyntaxNode. For those, we need to override Equals or wrap the SyntaxNode in a type that overrides Equals to use IsEquivalentTo on the SyntaxNode.

There are probably more places where we use SyntaxNode that aren't caught in the current tests.

Do you think it is worth filing an issue to do a pass over this?

@@ -32,19 +31,27 @@ public record InteropAttributeData
public InteropAttributeMember IsUserDefined { get; init; }
public bool SetLastError { get; init; }
public StringMarshalling StringMarshalling { get; init; }
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd add remarks to the InteropAttributeData doc about how it should not include any symbols / types that would keep a compilation alive.

@@ -18,6 +15,19 @@ public abstract record ManagedTypeInfo(string FullTypeName, string DiagnosticFor
private TypeSyntax? _syntax;
public TypeSyntax Syntax => _syntax ??= SyntaxFactory.ParseTypeName(FullTypeName);

public virtual bool Equals(ManagedTypeInfo other)
Copy link
Member

Choose a reason for hiding this comment

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

Should ManagedTypeInfo? be the parameter? Asking because the implementation checks for null.

Copy link
Member Author

Choose a reason for hiding this comment

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

There shouldn't be a null check, thanks for pointing that out!

@build-analysis build-analysis bot mentioned this pull request Dec 21, 2022
@jkoritzinsky
Copy link
Member

Only failure is #79874

@jkoritzinsky jkoritzinsky merged commit 55e0dea into dotnet:main Jan 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
5 participants