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

Record added node output states as new #61575

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Record added node output states as new #61575

merged 1 commit into from
Feb 3, 2023

Conversation

sbergen
Copy link
Contributor

@sbergen sbergen commented May 28, 2022

The (EntryState.Modified, EntryState.Added) => IncrementalStepRunReason.Modified state mapping was recently added in #61308, but it 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.

Here's an example test where I tried to use the node states, but ran into the missing switch cases (and found the PR adding them): https://github.com/sbergen/GenSubstitute/blob/TestImprovements/src/Tests/IncrementalGenerationTests.cs#L22-L33
Since there's a new source file being generated with the modified input, I would expect to see the status for that output as New.

@sbergen
Record added node output states as new
0a3e145 
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.
@sbergen sbergen requested a review from a team as a code owner May 28, 2022 18:02
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 28, 2022
@dnfadmin
Copy link

dnfadmin commented May 28, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

jcouv
jcouv approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv
Copy link
Member

jcouv commented Jan 27, 2023

@sbergen We've had some CI issues. Could you merge latest bits from the main branch into this PR? (I do something like git fetch origin main then git merge origin/main then push the commit)

@jcouv jcouv merged commit bfae7f9 into dotnet:main Feb 3, 2023
@ghost ghost added this to the Next milestone Feb 3, 2023
@jcouv
Copy link
Member

jcouv commented Feb 3, 2023

Thanks for the contribution @sbergen!

@DoctorKrolic
Copy link
Contributor

I don't know how, but it seems that after this PR some tests in main actually fail now due to these changes. This results in half of CI jobs failing in other PRs. Please fix

jjonescz added a commit that referenced this pull request Feb 3, 2023
@jjonescz
Revert "Record added node output states as new (#61575)"
0f0b5bd 
This reverts commit bfae7f9.
@jjonescz jjonescz mentioned this pull request Feb 3, 2023
Merged
jaredpar pushed a commit that referenced this pull request Feb 3, 2023
@jjonescz
Revert "Record added node output states as new (#61575)" (#66696)
391d367 
This reverts commit bfae7f9.
@jcouv
Copy link
Member

jcouv commented Feb 3, 2023 •
edited
Loading

This PR was reverted because it broke CI. I'm not sure the details.
@jjonescz Do we have any details on the failure?

@jjonescz
Copy link
Member

jjonescz commented Feb 6, 2023

GeneratorDriverTests were failing:

image

You should be able to reproduce these failures locally after merging main into this branch.

Note that the CI builds of this PR were outdated, they ran May 28, 2022.

@sbergen
Copy link
Contributor Author

sbergen commented Feb 6, 2023 •
edited
Loading

Hey, I haven't been able to get back to this recently, but will try to take a look during this week. I originally made the changes a long time ago, so I'm not too surprised if merging them now is causing some issues :)

333fred added a commit to 333fred/roslyn that referenced this pull request Feb 6, 2023
@333fred
Merge remote-tracking branch 'upstream/main' into vb-required
624934a 
* upstream/main: (547 commits)
  Add VerifyMethodBody helper as a replacement of VerifyIL (dotnet#66536)
  don't offer rename for ctor snippet (dotnet#66704)
  Get `ConditionalAttribute` type only once per compilation
  Convert language-specific option types to records (dotnet#66633)
  Enable MSBuild COMM log (dotnet#66708)
  ⚡️Share AssemblyloadTestFixture on Framework (dotnet#66684)
  NRT
  Don't overwrite binary logs in CI (dotnet#66683)
  Fix 'Generate Enum Member' to work in a 'Color Color' case.
  Find bitwise operators accessed through logical operators in FAR
  Semantic snippets - `propg` and `propi` snippets (dotnet#65979)
  NRT
  Properly classify aliases in quick info
  Fix
  Add tests
  Add support for finding collection initialiers with FAR
  Do not suggest replacing lambda with method group if the invoked mehod has `Conditional` attribute
  Revert "Record added node output states as new (dotnet#61575)" (dotnet#66696)
  Record added node output states as new (dotnet#61575)
  Fix formatting issue with convert-to-full-prop
  ...
@sbergen sbergen mentioned this pull request Feb 6, 2023
Merged
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@chsienki chsienki chsienki approved these changes

Assignees

@chsienki chsienki

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.6 P2
Development

Successfully merging this pull request may close these issues.
7 participants
@sbergen @dnfadmin @jcouv @DoctorKrolic @jjonescz @chsienki @RikkiGibson