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

LibraryImport with custom StringMarshalling type causes the source generator to keep the Compilation object alive #78242

Closed
jkoritzinsky opened this issue Nov 11, 2022 · 1 comment · Fixed by #79051
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Milestone

Comments

@jkoritzinsky
Copy link
Member

Description

When using the StringMarshallingCustomType property on a LibraryImport attribute, the generator caches a symbol object, causing the compilation to be kept alive across runs of the generator.

Reproduction Steps

I've included a test case (commented out) in this commit 32f856c (#77130)

Expected behavior

The test should pass.

Actual behavior

The test fails.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The problem is that the LibraryImportData record type contains a property for the StringMarshallingCustomType property from LibraryImportAttribute and we put that property into our "data model" type IncrementalStubGenerationContext:

We need to change how we pass the attribute data between the "compute the model" stage and the "generate code" stage such that the information we need is available, but the symbols are not passed while still having a decently understandable API.

@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

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

Issue Details

Description

When using the StringMarshallingCustomType property on a LibraryImport attribute, the generator caches a symbol object, causing the compilation to be kept alive across runs of the generator.

Reproduction Steps

I've included a test case (commented out) in this commit 32f856c (#77130)

Expected behavior

The test should pass.

Actual behavior

The test fails.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The problem is that the LibraryImportData record type contains a property for the StringMarshallingCustomType property from LibraryImportAttribute and we put that property into our "data model" type IncrementalStubGenerationContext:

We need to change how we pass the attribute data between the "compute the model" stage and the "generate code" stage such that the information we need is available, but the symbols are not passed while still having a decently understandable API.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 11, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 11, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Nov 11, 2022
@jkoritzinsky jkoritzinsky changed the title LibraryImport with custom StringMarshalling type causes the source generator to keep LibraryImport with custom StringMarshalling type causes the source generator to keep the Compilation object alive Nov 12, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 30, 2022
jkoritzinsky pushed a commit that referenced this issue Jan 3, 2023
…#79051)

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 ghost removed the in-pr There is an active PR which will close this issue when it is merged label 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
Development

Successfully merging a pull request may close this issue.

2 participants