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 generator incrementality #8388

Merged
merged 9 commits into from
Jan 4, 2023
Merged

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 14, 2022

Fixes #8386

Proposed changes

Customer Impact

Regression?

  • Yes / No

Risk

Screenshots

Before

After

Test methodology

Accessibility testing

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 14, 2022 19:20
@ghost ghost assigned Youssef1313 Dec 14, 2022
@@ -44,7 +45,7 @@ internal class ApplicationConfigurationGenerator : IIncrementalGenerator
return;
}

string? code = ApplicationConfigurationInitializeBuilder.GenerateInitialize(projectNamespace: GetUserProjectNamespace(syntaxNodes[0]), applicationConfig);
Copy link
Member Author

Choose a reason for hiding this comment

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

Collecting a bunch of nodes and using only the first node looks suspicious. I just kept the existing behavior, but definitely worth revisiting.

@dreddy-work
Copy link
Member

dreddy-work commented Dec 14, 2022

@Youssef1313 , i was about to ask on issue but can you please add description on these changes and why they are being made now in this PR?

@Youssef1313
Copy link
Member Author

@Youssef1313 , i was about to ask on issue but can you please add description on these changes and why they are being made now in this PR?

Currently, the generator pipeline has IncrementalValueProvider<Compilation> which breaks the whole thing. Every typed character in the IDE will execute the whole generator pipeline, which defeats the purpose of being "incremental". This PR makes the pipeline consists of only the things needed for generation.

RussKie
RussKie previously approved these changes Dec 14, 2022
@RussKie
Copy link
Member

RussKie commented Dec 14, 2022

@Youssef1313 while you're at it, do you think you could fully-qualify these types as well (to avoid issues similar to #7346):

string fontFamily = string.IsNullOrWhiteSpace(name)
? "Control.DefaultFont.FontFamily"
: $"new FontFamily(\"{name}\")";
return $"new Font({fontFamily}, {Size.ToString(CultureInfo.InvariantCulture)}f, (FontStyle){(int)Style}, (GraphicsUnit){(int)Unit})";

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Dec 14, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 15, 2022
@dreddy-work
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@elachlan
Copy link
Contributor

elachlan commented Jan 4, 2023

A couple of the ApplicationConfigurationGeneratorTests are failing.

HighDpiMode = PropertyDefaultValue.DpiMode,
UseCompatibleTextRendering = true
},
new ApplicationConfig(
Copy link
Member

Choose a reason for hiding this comment

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

@Youssef1313 , any specific reason to opt to change order of prams in tests instead of in record constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. It just happened that when I wrote the record, I matched the order here:

ApplicationConfig projectConfig = new()
{
EnableVisualStyles = enableVisualStyles,
DefaultFont = font,
HighDpiMode = highDpiMode,
UseCompatibleTextRendering = useCompatibleTextRendering
};
return (projectConfig, null);

as I was focusing more on the generator updates rather than tests.

@dreddy-work dreddy-work added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jan 4, 2023
@dreddy-work dreddy-work merged commit 639ca03 into dotnet:main Jan 4, 2023
@ghost ghost added this to the 8.0 Preview1 milestone Jan 4, 2023
@Youssef1313 Youssef1313 deleted the fix-incrementality branch January 4, 2023 18:42
@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
ready-to-merge PRs that are ready to merge but worth notifying the internal team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ApplicationConfigurationGenerator incremantality is broken
4 participants