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

Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LibraryImportGenerator). #71652

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

CyrusNajmabadi
Copy link
Member

Followup to #70911

Note: I'm going to add EventSourceGenerator (also in this project) in a followup to this PR.

@ghost
Copy link

ghost commented Jul 5, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Followup to #70911

Note: I'm going to add EventSourceGenerator (also in this project) in a followup to this PR.

Author: CyrusNajmabadi
Assignees: -
Labels:

area-System.Runtime.InteropServices

Milestone: -

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review July 5, 2022 17:52
@joperezr
Copy link
Member

joperezr commented Jul 5, 2022

cc @jkoritzinsky and @AaronRobinsonMSFT PTAL. For context, @CyrusNajmabadi is helping us add a much more performant way for source generators that depend on Attributes to run, and with these PRs he is onboarding generators one-by-one. Since you are the owners of the LibraryImportGenerator, do you mind taking a look at the changes and make sure it makes sense?

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Looks like we'll need to update the version of Roslyn that we test the LibraryImportGenerator against, but other than that LGTM

@CyrusNajmabadi
Copy link
Member Author

Looks like we'll need to update the version of Roslyn that we test the LibraryImportGenerator against,

@jkoritzinsky why would we need to do that? (from my perspective, the point of polyfilling was to help the runtime team not have to take a dependency on a new roslyn version). Thanks!

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Jul 5, 2022

The incremental tests are failing as they need the change in the generator infra to support the (Modified, Added) state transition for the step tracking. We shouldn't need to test the version that LibraryImportGenerator builds against, just what the tests run against.

@AaronRobinsonMSFT
Copy link
Member

We shouldn't need to test the version that LibraryImportGenerator builds against, just what the tests run against.

If we have to do this anyways, do we want the polyfill at all?

@CyrusNajmabadi
Copy link
Member Author

@jkoritzinsky any thoughts on #71652 (comment) ?

@joperezr for context, Roslyn4 has an actual bug in it which can't be fixed in the polyfill layer. It's unclear to me how we intend to get that fix out to your layer such that you can consume it.

@jkoritzinsky
Copy link
Member

Given that we should be able to consume the new Roslyn soon (within the month?) it might be okay to wait. But if we're causing enough perf issues to show up in VS traces, then it might be worthwhile to take the polyfill.

@jkoritzinsky
Copy link
Member

I just took a quick look at things and found out something useful:

Arcade's minimum SDK version is 7.0.0 Preview 5. That means that we can update our Roslyn version that we ship the generators with (the MicrosoftCodeAnalysisVersion_4_X property) to 4.3.0-2.22307.7, which contains the bug fix for the Roslyn bug this PR is hitting.

This still won't let us use the new API Cyrus is polyfilling here (as that is going to be in the next preview).

I think it's definitely worthwhile to update the Roslyn version we're building these tests against to the version above, and I think it's probably worth it to polyfill the API if it will give us big performance improvements.

@CyrusNajmabadi
Copy link
Member Author

@jkoritzinsky can you help with the change you want to see made here? I'm not entirely sure where i should make it. Thanks!

…to the public version that matches the SDK that arcade's global.json is referencing (also the public version used by VS 17.3 Preview 2)
@jkoritzinsky
Copy link
Member

Made the change! I figured out that the version I shared is the same as the most recent published version (built from the same commit), so I used the "public" package version instead of the one I shared previously.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the failing tests (which sound like they're a known issue with us waiting on a new version?), LGTM.

@CyrusNajmabadi
Copy link
Member Author

@jkoritzinsky NativeTypeWithConstructorAndFromNativeValueMethod_ReportsDiagnostic (not related to generators afaict) failed on CI. Running locally it passes. Do you know what the issue might be here?

@jkoritzinsky
Copy link
Member

I’m not sure. I’ll take a look tomorrow. If it’s blocking, you can disable the test. We’re going to be rewriting that analyzer very soon, so the existing tests aren’t particularly useful.

@CyrusNajmabadi
Copy link
Member Author

I’m not sure. I’ll take a look tomorrow. If it’s blocking, you can disable the test. We’re going to be rewriting that analyzer very soon, so the existing tests aren’t particularly useful

I can do that if you're sure. I am a bit concerned that this is due to the roslyn update, and that somehow may be breaking you in some way. I'd def love if someone can actually check to see what's going on so there isn't some unintentional regression.

@CyrusNajmabadi
Copy link
Member Author

@jkoritzinsky can you help here? i think this is hte last day we can get this in. @joperezr as well.

@jkoritzinsky
Copy link
Member

I'm heads-down on getting other work in for LibraryImportGenerator before the code-complete deadline. I don't think I'll have bandwidth to get to this today.

@CyrusNajmabadi
Copy link
Member Author

@jkoritzinsky @joperezr is there someone else we can pull in? Today is the last day to get this in. So if we miss this it won't happen till the next release.

@stephentoub
Copy link
Member

stephentoub commented Jul 11, 2022

Today is the last day to get this in.

This is an important perf fix. It's fine to go beyond tomorrow for .NET 7. (But sooner the better.) If we don't get it in tomorrow, it'll just miss P7.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

The change LGTM

The test failure is due to the change in Roslyn to prefer nint over IntPtr when adding a method that has IntPtr in the signature. This failure is fine as we were using the "Preview" langversion so we understand that these sorts of changes happen when we update Roslyn versions.

@joperezr
Copy link
Member

joperezr commented Jul 13, 2022

/azp run runtime

Restarting the runtime CI since the failing tests have now been removed, so just ensuring that we have a green CI before merging.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 13, 2022
@joperezr
Copy link
Member

The remaining JIT failure is unrelated, so I'll go ahead and merge this.

@joperezr joperezr merged commit 92d8a8b into dotnet:main Jul 13, 2022
@joperezr
Copy link
Member

/backport to release/7.0-preview7

@github-actions
Copy link
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2666797473

@github-actions
Copy link
Contributor

@joperezr backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LibraryImportGenerator).
Applying: Move common code to shared location
Using index info to reconstruct a base tree...
M	src/libraries/System.Text.RegularExpressions/gen/System.Text.RegularExpressions.Generator.csproj
A	src/libraries/System.Text.RegularExpressions/src/System/Collections/Generic/ValueListBuilder.Pop.cs
Falling back to patching base and 3-way merge...
CONFLICT (rename/rename): Rename src/libraries/System.Text.RegularExpressions/src/System/Collections/Generic/ValueListBuilder.Pop.cs->src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.Pop.cs in HEAD. Rename src/libraries/System.Runtime.InteropServices/src/System/Collections/Generic/ValueListBuilder.Pop.cs->src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.Pop.cs in Move common code to shared location
Auto-merging src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.Pop.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Move common code to shared location
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

joperezr pushed a commit to joperezr/runtime that referenced this pull request Jul 13, 2022
…roslyn (for LibraryImportGenerator). (dotnet#71652)

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
carlossanlop pushed a commit that referenced this pull request Jul 14, 2022
…roslyn (for LibraryImportGenerator). (#71652) (#72142)

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

Co-authored-by: CyrusNajmabadi <cyrusn@microsoft.com>
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants