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.

Sign up for GitHub

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

Jump to bottom

Make the EventSource generator incremental et.al. #64579

Merged
merged 8 commits into from
Mar 18, 2022
Merged

Make the EventSource generator incremental et.al. #64579

merged 8 commits into from
Mar 18, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

The EventSource generator was made incremental and its output is marked as auto-generated. (TIL GitHub doesn't let me reopen a PR if I have force-pushed its branch)

Fixes #64563.

c.c. @danmoseley

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 31, 2022
@ghost
Copy link

ghost commented Jan 31, 2022

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

The EventSource generator was made incremental and its output is marked as auto-generated. (TIL GitHub doesn't let me reopen a PR if I have force-pushed its branch)

Fixes #64563.

c.c. @danmoseley

Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Diagnostics.Tracing
Milestone: -

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Feb 7, 2022
@teo-tsirpanis
Copy link
Contributor Author

I updated the generator to only use the semantic model provided by CreateSyntaxProvider's transform delegate, completely removing the need for injecting the Compilation into the pipeline, and drastically simplifying it.

Can you review it again?

chsienki
chsienki approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

You'll want to implement IEquatable<> on the EventSourceClass to ensure you don't do duplicate work unnecessarily, but otherwise LGTM.

sharwell
sharwell reviewed Feb 8, 2022
View reviewed changes
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This approach is much better! So glad to see the compilation issue resolved 👍

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs Outdated Show resolved Hide resolved
@sharwell sharwell dismissed their stale review February 8, 2022 00:07

The problematic compilation-ignoring comparer has been eliminated in favor of clean data extraction.

@teo-tsirpanis
Copy link
Contributor Author

All PR feedback was addressed.

For the record, source generators (especially the incremental ones) are awesome.

@teo-tsirpanis teo-tsirpanis mentioned this pull request Feb 16, 2022
Merged
@tommcdon
Copy link
Member

tommcdon commented Mar 7, 2022

@stephentoub @noahfalk @hoyosjs

@tommcdon
Copy link
Member

@stephentoub @noahfalk @hoyosjs just a friendly ping, any concerns with moving forward with this change?

noahfalk
noahfalk approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

I only skimmed, but LGTM. Thanks!

@danmoseley
Copy link
Member

Is this good to merge now?

@chsienki
Copy link
Contributor

@danmoseley Looks good from the compiler side, modulo the revert of GetBestTypeByMetadataName. Not a blocker, but as discussed on the regex generator PR it can be a subtle breaking change so suggest switching it back.

@teo-tsirpanis
Copy link
Contributor Author

OK, I reverted to GetBestTypeByMetadataName.

@danmoseley
Copy link
Member

@noahfalk @hoyosjs is this mergeable (after build break is fixed..)

@teo-tsirpanis
Copy link
Contributor Author

Build what? Oh crap, I had built it from VS and it worked. I will take a look soon. 😥

@teo-tsirpanis
Copy link
Contributor Author

Turns out #66434 changed the location of IsExternalInit.cs. It builds on my machine after a rebase. Let's see now...

@teo-tsirpanis
Copy link
Contributor Author

Ahhh, what went wrong this time?

@chsienki I removed GetBestTypeByMetadataName. This generator is only internally used by corelib, so there is no need for keeping backwards compatibility over an edge-case scenario.

@noahfalk
Copy link
Member

@noahfalk @hoyosjs is this mergeable (after build break is fixed..)

Yep!

@danmoseley danmoseley merged commit 4d39501 into dotnet:main Mar 18, 2022
@danmoseley
Copy link
Member

Thanks @teo-tsirpanis !

@teo-tsirpanis teo-tsirpanis deleted the eventsource-gen-incremental branch March 18, 2022 12:53
@teo-tsirpanis
Copy link
Contributor Author

BTW are you still on vacation @noahfalk? Perhaps you forgot to clear your GitHub profile status.

@noahfalk
Copy link
Member

nope I've been back a good while, thanks for the heads up : )

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@teo-tsirpanis @radekdoulik
Make the EventSource generator incremental et.al. (dotnet#64579)
80e678a
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@sharwell sharwell sharwell left review comments

@chsienki chsienki chsienki approved these changes

@noahfalk noahfalk noahfalk approved these changes

Assignees
No one assigned
Labels
area-System.Diagnostics.Tracing community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EventSource source generator should mark both source and IL as autogenerated
7 participants
@teo-tsirpanis @tommcdon @danmoseley @chsienki @noahfalk @sharwell @stephentoub