[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach

[joperezr]

Fixes #70754

Description

This PR is upgrading all of the 7.0 inbox source generators (Json one is pending and I will add a commit that addresses that one too in a bit) to use Roslyn version 4.4 (version picked is the roslyn included in SDK 7.0.100-rc.1.22425.9), 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. Additionally, this PR is also moving the repo to use that same roslyn version for the compilers used to build the repo.

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 1 or older.
• Manually referencing Microsoft.NET.Compilers.Toolset package version 4.4.0-2.22423.18 or older

Regression

No.

Risk

Medium, since we are essentially changing the compilers that get used to build all of the repo, however, note that this needs to be done anyway since we want to ship .NET 7 with pretty much the final version of the compilers. 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.

[ghost]

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

Issue Details

---

Fixes #70754

This PR is upgrading all of the 7.0 inbox source generators (Json one is pending and I will add a commit that addresses that one too in a bit) to use Roslyn version 4.4 (version picked is the roslyn included in SDK 7.0.100-rc.1.22425.9), 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. Additionally, this PR is also moving the repo to use that same roslyn version for the compilers used to build the repo.

After the Json source generator is also ported, the only one left will be the Logging generator, which I believe only ships OOB. @eerhardt do we want to also cross compile against roslyn4.4 in that case and ship in the package? or do we want to use roslyn4.4 instead of the current 4.0? Once that is clear, I will go ahead and add to this PR both the Json source generator and Logging generator changes to this PR.

Finally, After this PR gets merged, upstack repos will be broken unless they start using this roslyn version (or greater) to build. For this reason, the plan is to, before merging this PR, merge a PR in dotnet/arcade that moves both the SDK and the Roslyn version up for all repos, and that way, after this goes in, upstack repos won't be broken.

Adding NoMerge label for now while the Json and Logging generators get ported, and the arcade changes get ready, but sending out the PR now to get early feedback.

Author:	joperezr
Assignees:	-
Labels:	NO-MERGE, area-Meta
Milestone:	-

[joperezr]

FYI: @jaredpar @marcpopMSFT

[CyrusNajmabadi]

Done with pass.

[carlossanlop]

@danmoseley need an approval please.

[joperezr]

need an approval please

Getting approval is fine now, but please do note that this is not yet ready to go in.

[carlossanlop]

Getting approval is fine now, but please do note that this is not yet ready to go in.

Apologies. I missed the NO MERGE label.

[danmoseley]

This one is worth filling out the template for also. Eg., explaining how this is more than just code cleanup without visible effects.

. For this reason, the plan is to, before merging this PR, merge a PR in dotnet/arcade that moves both the SDK and the Roslyn version up for all repos, and that way, after this goes in, upstack repos won't be broken.

I assume we want to do that anyway, so that we ultimately ship having built with the shipping SDK, more or less.

[ViktorHofer]

One of the questions that Tactics might raise is why this isn't in main yet. Would it make sense to drive the required changes (including the SDK update) into main first?

[joperezr]

Main is already on a newer version of roslyn, that said it doesn't yet target it for the source generators specifically. I can certainly add a PR for that too.

[danmoseley]

Yes main first would be good - although, perhaps it would take some time before it flowed through and was testable in VS, so that would not increase coverage signficantly anytime soon (?)

[carlossanlop]

There's a merge conflict FYI.

[marcpopMSFT]

How will this work if someone is using the 7.0.100 SDK in 17.3 to target 6.0 apps? Will the source generators only be used when targeting 7.0 applications? (Reminder that we're limiting 7.0.100 to 17.3 and above and 7.0.200 will be 17.4 and up).

[joperezr]

the new source generators that depend on roslyn 4.4 ship as part of the 7.0 targeting pack, so they will only be referenced if you are actually targeting .NET 7.0. Even if you use the new SDK, If you are targeting net6.0 then these generators should never be passed to the compiler.

[joperezr]

This is now ready for review.

@danmoseley I have filled the template now in the main description.

[joperezr]

Once PR dotnet/arcade#10724 in arcade gets merged, all upstack repos will get an sdk bump and compiler bump automatically, which will allow them to ingest this change.

[carlossanlop]

Once PR dotnet/arcade#10724 in arcade gets merged, all upstack repos will get an sdk bump and compiler bump automatically, which will allow them to ingest this change.

Ping me when ready to merge, please, @joperezr .

[danmoseley]

Approved -- we don't want to ship the way we are, this brings us into a regular state. Of course please watch the bits after they go in to ensure we spot any unexpected issue early.

[carlossanlop]

There's a merge conflict. Can one of you please resolve it, @CyrusNajmabadi or @joperezr?

[joperezr]

Yes, I'm working on this now. Turns out that the version that got merged into Arcade of the SDK had a different problem that I only discovered, will quickly push a PR to fix that, and then will go ahead and fix the changes here and merge

[joperezr]

@carlossanlop conflict is resolved and this should be ready to go. Feel free to merge as soon as CI passes 👍

[lewing]

  Starting:    JIT.HardwareIntrinsics.XUnitWrapper (parallel test collections = on, max threads = 2)
    JIT/HardwareIntrinsics/Arm/Sha256/Sha256_r/Sha256_r.sh [FAIL]
      Unhandled exception: System.IO.IOException: Connection timed out : 'Global\msbuild-server-launch-gkQydOqVnpHlKzivWKBFxHKmWmVPqCBlomIg710sl38'
         at System.Threading.Mutex.CreateMutexCore(Boolean initiallyOwned, String name, Boolean& createdNew)
         at Microsoft.Build.Experimental.MSBuildClient.TryLaunchServer()
         at Microsoft.Build.Experimental.MSBuildClient.Execute(CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildClientApp.Execute(String[] commandLine, String msbuildLocation, CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildApp.Main(String[] args)
         at Microsoft.DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.ExecuteInProc(String[] arguments)
         ```

[joperezr]

Yes, I'm working with @tannergooding to see if this is caused by the SDK update.

[carlossanlop]

@joperezr merging as requested.