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

[release/7.0-rc2] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #75875

Merged

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Sep 20, 2022

Backport of #75717 to release/7.0-rc2

/cc @CyrusNajmabadi @jkoritzinsky @stephentoub

Description

This PR is upgrading all of the 7.0 inbox source generators to use Roslyn version 4.4, which allows us to remove the polyfill approach from them and instead use the inbox Roslyn APIs for finding an attribute in a performant manner.

We originally tried applying these changes in PR #74822 before the RC2 snap, but it caused issues when ingesting the change in the sdk repo since we didn't have a prerelease version of the SDK with the compilers that we required in order to do so. Now that RC1 has been released, this shouldn't be a problem any longer.

Customer Impact

After this change goes in, the only way to consume dotnet/runtime (without warnings or disabled analyzers) will be by using a Roslyn4.4 compiler. These source generators are on by default when targeting dotnet 7.0, so all projects automatically pass them in to the compiler. Since these source generators will now depend on Roslyn 4.4, then the consuming app will need to use the Roslyn 4.4 compilers too, which can be done via using any of the following:

  • Building using an SDK which is .NET 7 RC1 or older.
  • Building using Visual Studio 17.4 preview 2 or older.
  • Manually referencing Microsoft.NET.Compilers.Toolset package version 4.4.0-2.22423.18 or older

Risk

Low/Medium: since we need this change to be ingested upstack and released into an SDK in order to fully test End-To-End. This is also in a sense reducing risk, as it is removing from the inbox source generators the dependency to Roslyn API that was not fully tested and only source-copied. After this, we will now instead depend on the Roslyn tested bits.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned joperezr Sep 20, 2022
@carlossanlop
Copy link
Member

@joperezr please fill out the template, add servicing-consider, and send the email to Tactics.

@carlossanlop carlossanlop added the source-generator Indicates an issue with a source generator feature label Sep 20, 2022
@joperezr joperezr added the Servicing-consider Issue for next servicing release review label Sep 20, 2022
@ghost
Copy link

ghost commented Sep 20, 2022

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

Issue Details

Backport of #75717 to release/7.0-rc2

/cc @CyrusNajmabadi @jkoritzinsky @stephentoub

Description

This PR is upgrading all of the 7.0 inbox source generators to use Roslyn version 4.4, which allows us to remove the polyfill approach from them and instead use the inbox Roslyn APIs for finding an attribute in a performant manner.

We originally tried applying these changes in PR #74822 before the RC2 snap, but it caused issues when ingesting the change in the sdk repo since we didn't have a prerelease version of the SDK with the compilers that we required in order to do so. Now that RC1 has been released, this shouldn't be a problem any longer.

Customer Impact

After this change goes in, the only way to consume dotnet/runtime (without warnings or disabled analyzers) will be by using a Roslyn4.4 compiler. These source generators are on by default when targeting dotnet 7.0, so all projects automatically pass them in to the compiler. Since these source generators will now depend on Roslyn 4.4, then the consuming app will need to use the Roslyn 4.4 compilers too, which can be done via using any of the following:

  • Building using an SDK which is .NET 7 RC1 or older.
  • Building using Visual Studio 17.4 preview 2 or older.
  • Manually referencing Microsoft.NET.Compilers.Toolset package version 4.4.0-2.22423.18 or older

Risk

Low/Medium: since we need this change to be ingested upstack and released into an SDK in order to fully test End-To-End. This is also in a sense reducing risk, as it is removing from the inbox source generators the dependency to Roslyn API that was not fully tested and only source-copied. After this, we will now instead depend on the Roslyn tested bits.

Author: joperezr
Assignees: joperezr
Labels:

Servicing-consider, area-Meta, source-generator

Milestone: -

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 20, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Sep 20, 2022

JIT failures are a known issue in wasm: #75391
Approved, signed off, ready to merge. :shipit:

@carlossanlop carlossanlop merged commit ad9abbe into dotnet:release/7.0-rc2 Sep 20, 2022
carlossanlop added a commit that referenced this pull request Sep 26, 2022
* Fix querying L3 cache size on osx-x64 (#75870)

Co-authored-by: Filip Navara <[email protected]>

* KeyChar should be preserved for Ctrl+Letter (#75861)

Co-authored-by: Adam Sitnik <[email protected]>

* Try re-enabling System.Transactions.Local tests on Arm64 (#75703)

Co-authored-by: Jan Kotas <[email protected]>

* Updating inbox source generators to Roslyn 4.4 and removing polyfill approach (#75717) (#75875)

* Removed internalProperties group from proxy and tests. (#75906)

Co-authored-by: Ilona Tomkowicz <[email protected]>

* [release/7.0-rc2] Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs (#75885)

* Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs

* Apply suggestions from code review

Co-authored-by: Adam Sitnik <[email protected]>

* Adjusting the comment as per PR review feedback

Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>

* Bump intellisense package for RC2 to include latest comments for Numerics (#75938)

Co-authored-by: carlossanlop <[email protected]>

* [release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions (#75996)

* bring back the old code...

* bring back more old code

* Use an ifdef around clr code instead of a separate file

* Delete SpanHelpers.Clr.cs

* Remove a remaining INumber<T> helper from mono

Co-authored-by: Adam Sitnik <[email protected]>
Co-authored-by: Jeff Handley <[email protected]>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Filip Navara <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>
Co-authored-by: Jan Kotas <[email protected]>
Co-authored-by: Jose Perez Rodriguez <[email protected]>
Co-authored-by: Ilona Tomkowicz <[email protected]>
Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Carlos Sanchez <[email protected]>
Co-authored-by: carlossanlop <[email protected]>
Co-authored-by: Jeff Handley <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Servicing-approved Approved for servicing release source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants