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

Doc Cookbook Auto Interface Implementation #73906

Merged
merged 21 commits into from
Jul 1, 2024

Conversation

bdbonazz
Copy link
Contributor

@bdbonazz bdbonazz commented Jun 8, 2024

Added documentation of IncrementalGenerators Cookbook about Auto interface implementation

#72149

Working solution: https://github.com/bdbonazz/AutoImplementGenerator

Added documentation of IncrementalGenerators
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 8, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 8, 2024
@bdbonazz
Copy link
Contributor Author

bdbonazz commented Jun 8, 2024

@dotnet-policy-service agree

docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
@jaredpar
Copy link
Member

@333fred, @CyrusNajmabadi PTAL

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Unfortunately, this type of generator is not something that we want to put in the cookbook or recommend that people do. Going through all interfaces implemented by a type, particularly the way that ends up being implemented here (using a Compilation as an incremental parameter) is not actually incremental in any fashion and will kill IDE performance. As of yet, we have no ideas for how to implement this type of generator efficiently enough to suggest that people should write a generator in this fashion.

A restructuring of the generator that attributes the types that get their interfaces auto-implemented, rather than the interfaces themselves, would be acceptable to include. But the current structure is something that we explicitly recommend against doing in this very document.

@bdbonazz
Copy link
Contributor Author

@333fred
I agree with you about performance issues, and I agree with @sharwell about using "ForAttributeWithMetadataName "
Do you think a solution of this type would be acceptable?

[AutoImplement(nameof(IUserInterface), nameof(IUserInterface2))]
public partial class UserClass : IUserInterface, IUserInterface2
{
    public string UserProp { get; set; }
}

Moving the attribute to the class would let me use "ForAttributeWithMetadataName" to get only the decorated classes.
I would no longer cycle through all the interfaces of a class, but only the interfaces passed as an argument to the attribute.
Bonus point: the developer could decide when a class should auto-implement an interface and when to implement the interface manually

If at least one of you gives me the ok, I will start implementing this solution

@333fred
Copy link
Member

333fred commented Jun 11, 2024

Yes, that's exactly the type of structure I meant when I said a change could be acceptable to include.

@bdbonazz
Copy link
Contributor Author

@333fred
Perfect
Just one last thing:
You rightly had doubts about "using a Compilation as an incremental parameter".
Still I think I need Compilation in order to get INamedTypeSymbol interfaceSymbol from the interface name (and I need interfaceSymbol in order to have interfaceUsings, interfaceProperties, etc.)
I don't know if there is a more efficient way to do this.
Would you prefer I get the interface infos in the RegisterSourceOutput method like I'm doing now? Or should I get that infos in the transform argument of the ForAttributeWithMetadataName method?

@333fred
Copy link
Member

333fred commented Jun 11, 2024

That info is already available in the context of your transform callback in FAWMN. See https://github.com/dotnet/roslyn/blob/main/docs/features/incremental-generators.cookbook.md#augment-user-code.

@bdbonazz
Copy link
Contributor Author

@333fred You are right, now it should be more performant

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass. Large step in the right direction, but there's still plenty of room for improvement.

docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

@CyrusNajmabadi @chsienki for reviews as well please.

docs/features/incremental-generators.cookbook.md Outdated Show resolved Hide resolved
@333fred 333fred merged commit 4e35469 into dotnet:main Jul 1, 2024
5 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 1, 2024
@333fred
Copy link
Member

333fred commented Jul 1, 2024

Thanks @bdbonazz!

@bdbonazz
Copy link
Contributor Author

bdbonazz commented Jul 1, 2024

Thanks @bdbonazz!
@333fred
You are welcome
Thank you too, I learned a lot from your comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants