Record added node output states as new #66725

sbergen · 2023-02-06T20:37:23Z

Retake of #61575, which spent a long time in review limbo.

The (EntryState.Modified, EntryState.Added) => IncrementalStepRunReason.Modified state mapping was added in #61308, but it doesn't look correct to me. It was even suggested in the original PR discussion that this should be New.

However, there were several new tests added since I originally wrote this (which is why merging it caused some CI havoc), and I pretty much blindly modified those tests to match this, as I had a bit of trouble following the intent in these tests. Would be great to verify that the changes look good!

Also, now that I was revisiting the code, I reviewed the documentation in IncrementalStepRunReason, and updated it based on my understanding of what would make sense. However, it made me wonder: Since NodeStateTable.AsStepState has seven entries, should IncrementalStepRunReason also have seven? We could discriminate between "added from new input" vs "added from modified input" and similar states for removal.

The `(EntryState.Modified, EntryState.Added) => IncrementalStepRunReason.Modified` state mapping was recently added in dotnet#61308, but doesn't look correct to me. Since this value is used as the output status, and the outputs are new, I believe the correct state should be `IncrementalStepRunReason.New` instead.

jcouv · 2023-02-07T18:56:19Z

src/Compilers/Core/Portable/SourceGeneration/IncrementalStepRunReason.cs

@@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis
    public enum IncrementalStepRunReason
    {
        /// <summary>
-        /// The output of this step is a new output produced from a new input.
+        /// The input to this step was added or modified from a previous run, and it produced a new output.


This is still not very clear. There's some ambiguity between an input node and the entries that it output.
@chsienki Would it make sense to simplify (here and below) to: "The input to this step produced a new output."

I tried to follow the same format as the other enum values here. E.g. "The input to this step was modified from a previous run" is used twice below, for Modified and Unchanged. I personally find the symmetry between the documentation strings useful for understanding them.

I don't really get what could be considered ambiguous here, but it could be that I'm just misunderstanding something.

jcouv

Done with review pass (iteration 4). Minor question only on doc comments

jcouv · 2023-03-09T05:06:39Z

@chsienki Please take a look. Thanks

jcouv · 2023-04-12T23:01:42Z

@chsienki Please take a look. Thanks

chsienki

I think this is fine, but it does bring up an odd issue, that this is actually a breaking change. Users have tests that assert Modified will start failing with a newer compiler version.

/cc @jaredpar for his thoughts on that.

jaredpar · 2023-04-24T21:35:50Z

@chsienki

I'm fine with this as a breaking change. Testing generators at this level is a very advanced scenario. Customers that are doing this are likely to appreciate this being more correct here. The items that are important to me:

Do we have a concise example of the type of change that produces this behavior?
Is this the correct change? Seems general agreement yes. Want to hear @jjonescz thoughts too. If we all agree though then let's figure out "how" to dot this.

sbergen · 2023-04-25T07:35:08Z

Do we have a concise example of the type of change that produces this behavior?

This is not concise from the perspective of complete implementation, but it's how I ran into this issue, and is rather concise in concept.

I have a mocking library: Let's start with the test. It's testing that when I add a new mock creation call for a new type, the run reason for the new type should be New (and it currently says Modified).

The logic in the generator is the following:

Collect all mock creation calls (extracting the generic argument)
Filter down to unique types
Generate mock classes for each type

So the key here is that there's a list of type models which changes, but items added to the list are currently giving the "modified" status.

jjonescz · 2023-04-25T08:29:40Z

src/Compilers/Core/Portable/SourceGeneration/Nodes/NodeStateTable.cs

@@ -406,7 +406,7 @@ private static IncrementalStepRunReason AsStepState(EntryState inputState, Entry
                    (EntryState.Cached, EntryState.Cached) => IncrementalStepRunReason.Cached,
                    (EntryState.Removed, EntryState.Removed) => IncrementalStepRunReason.Removed,
                    (EntryState.Modified, EntryState.Removed) => IncrementalStepRunReason.Removed,
-                    (EntryState.Modified, EntryState.Added) => IncrementalStepRunReason.Modified,
+                    (EntryState.Modified, EntryState.Added) => IncrementalStepRunReason.New,


Should we document the breaking change somewhere? Seems like this is the place, but not sure it's actually used: https://github.com/dotnet/roslyn/blob/main/docs/Breaking%20API%20Changes.md

I added a new "Unreleased" section to that doc, as I'm not sure how your versioning works. If there's a concrete version number I should add, I can change that.

jjonescz · 2023-04-25T08:36:16Z

.../CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests_Attributes_FullyQualifiedName.cs

@@ -1580,7 +1580,7 @@ class XAttribute : System.Attribute
        Assert.Equal(IncrementalStepRunReason.Unchanged, runResult.TrackedSteps["collectedGlobalAliases_ForAttribute"].Single().Outputs.Single().Reason);
        Assert.Equal(IncrementalStepRunReason.Cached, runResult.TrackedSteps["compilationGlobalAliases_ForAttribute"].Single().Outputs.Single().Reason);
        Assert.Equal(IncrementalStepRunReason.Cached, runResult.TrackedSteps["allUpGlobalAliases_ForAttribute"].Single().Outputs.Single().Reason);
-        Assert.Equal(IncrementalStepRunReason.Modified, runResult.TrackedSteps["compilationUnit_ForAttribute"].Single().Outputs.Single().Reason);
+        Assert.Equal(IncrementalStepRunReason.New, runResult.TrackedSteps["compilationUnit_ForAttribute"].Single().Outputs.Single().Reason);


Do we have a concise example of the type of change that produces this behavior?

This test looks like a concise example. We have generator FAWMN(XAttribute), start with class C { }, modify it to [X] class C { }, and the source generator produces new output for the class C due to modified input (adding attribute X). Previously, this would give IncrementalStepRunReason.Modified, now it gives IncrementalStepRunReason.New.

jcouv · 2023-05-09T19:16:10Z

@sbergen This is almost ready to merge. Only a couple of outstanding questions. Thanks

sbergen · 2023-05-10T07:17:00Z

@sbergen This is almost ready to merge. Only a couple of outstanding questions. Thanks

Let's make sure we ensure that CI is green with changes from main merged in this time 😅

jcouv · 2023-05-10T14:59:15Z

@sbergen Yup. I'll wait for you to address the couple remaining issues/questions, then re-run CI.

sbergen · 2023-05-10T16:52:36Z

@sbergen Yup. I'll wait for you to address the couple remaining issues/questions, then re-run CI.

@jcouv Oh, sorry, I didn't realise you were waiting for my input still.

I can see the breaking changes entry as a concrete thing to do, but I didn't find any other feedback actionable. Should I add an entry for 4.7.0 into the breaking changes doc? I have no clue what your release process looks like :)

…p-added-output

jcouv · 2023-05-12T18:02:38Z

Thanks @sbergen for the contribution!

cabauman · 2023-05-17T10:05:42Z

Customers that are doing this are likely to appreciate this being more correct here.

I'm one of them. Thanks @sbergen!

sbergen added 3 commits February 6, 2023 20:49

Update tests that were added since I originally implemented the change

16a2862

Update documentation for IncrementalStepRunReason

ce1ab42

dotnet-issue-labeler bot added the Area-Compilers label Feb 6, 2023

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 6, 2023

Update VB tests also

63b5ed4

sbergen marked this pull request as ready for review February 7, 2023 07:09

sbergen requested a review from a team as a code owner February 7, 2023 07:09

jcouv reviewed Feb 7, 2023

View reviewed changes

jcouv self-assigned this Feb 7, 2023

jcouv requested review from jcouv and chsienki February 8, 2023 18:28

jaredpar added the New Feature - Source Generators Source Generators label Mar 31, 2023

chsienki approved these changes Apr 24, 2023

View reviewed changes

jjonescz approved these changes Apr 25, 2023

View reviewed changes

jjonescz reviewed Apr 25, 2023

View reviewed changes

sbergen added 2 commits May 10, 2023 20:16

Merge remote-tracking branch 'upstream/main' into reapply-fix-run-ste…

c681f6d

…p-added-output

Add note about breaking changes with IncrementalStepRunReason

65e2a3c

jcouv removed their assignment May 10, 2023

jjonescz self-assigned this May 11, 2023

Merge branch 'main' into sbergen/reapply-fix-run-step-added-output

f215f3c

jjonescz enabled auto-merge (squash) May 12, 2023 08:21

jjonescz merged commit 02cf37e into dotnet:main May 12, 2023

ghost added this to the Next milestone May 12, 2023

allisonchou mentioned this pull request May 12, 2023

[Automated] PRs inserted in VS build main-33712.304 #68184

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record added node output states as new #66725

Record added node output states as new #66725

sbergen commented Feb 6, 2023

jcouv Feb 7, 2023

sbergen May 10, 2023

jcouv left a comment

jcouv commented Mar 9, 2023

jcouv commented Apr 12, 2023

chsienki left a comment

jaredpar commented Apr 24, 2023

sbergen commented Apr 25, 2023

jjonescz Apr 25, 2023

sbergen May 10, 2023

jjonescz Apr 25, 2023 •

edited

Loading

jcouv commented May 9, 2023

sbergen commented May 10, 2023

jcouv commented May 10, 2023

sbergen commented May 10, 2023

jcouv commented May 12, 2023

cabauman commented May 17, 2023

Record added node output states as new #66725

Record added node output states as new #66725

Conversation

sbergen commented Feb 6, 2023

jcouv Feb 7, 2023

Choose a reason for hiding this comment

sbergen May 10, 2023

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Mar 9, 2023

jcouv commented Apr 12, 2023

chsienki left a comment

Choose a reason for hiding this comment

jaredpar commented Apr 24, 2023

sbergen commented Apr 25, 2023

jjonescz Apr 25, 2023

Choose a reason for hiding this comment

sbergen May 10, 2023

Choose a reason for hiding this comment

jjonescz Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv commented May 9, 2023

sbergen commented May 10, 2023

jcouv commented May 10, 2023

sbergen commented May 10, 2023

jcouv commented May 12, 2023

cabauman commented May 17, 2023

jjonescz Apr 25, 2023 •

edited

Loading