dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

Remove lightup approach for newer [InterceptorAttribute] feature; need to know compiler version #104693

Closed steveharter closed 4 weeks ago

steveharter commented 1 month ago

Support for the newer [InterceptorAttribute] feature was added to the runtime in https://github.com/dotnet/runtime/issues/101079. However, that does a "lightup" approach for v9 which means reflection is used to call into the new Roslyn APIs. This will be ported to v8 and remain that way, but for v9 we should remove the older approach and possibly the lightup code altogether and use a direct reference to the new APIs. In order to do that, we need the minimum version of the compiler, SDK and Visual Studio that supports this feature -- what are those? We also have to communicate that to any down-level consumers including the source-build approach and build machine environment.

Currently the runtime repo requires 4.8.0 for source generators but as mentioned there, we should never be greater than the latest public version in VS or SDK. That 4.8.0 compiler version does not include the new feature, but it appears the currently referenced SDK from the runtime repo does (I believe it references 4.11.0.* of the compiler) so that's why the lightup approach in v9 is pulling in the new feature.

cc @jaredpar @marcpopMSFT @ericstj

dotnet-policy-service[bot] commented 1 month ago

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

ericstj commented 1 month ago

Another way to state this - @jaredpar / @marcpopMSFT - what's the minimum version of the compiler guaranteed to be present in both VS and the dotnet CLI for projects targeting net9.0? Could you walk us through how to determine this?

marcpopMSFT commented 1 month ago

Follow the minimum VS version as listed here: https://learn.microsoft.com/en-us/dotnet/core/porting/versioning-sdk-msbuild-vs#targeting-and-support-rules For 9.0.100, that will be 17.11.

That being said, if we go with this new proposal (which is POR for net9), I don't think the minimum version requirement for roslyn dependencies would apply anymore: https://github.com/dotnet/sdk/pull/41790 That's assuming the component this is used it ends up being dual inserted into VS and used from VS.

ericstj commented 1 month ago

We'd still need to build for some minimum version that's in VS, we wouldn't have any older codebase for these target-framework-specific analyzers.

Sounds like 17.11 is the version we can depend on, and according to https://github.com/dotnet/roslyn/issues/72133 it went into 17.11 - cc @RikkiGibson.

So it would seem that we could update the roslyn version referenced by inbox analyzers to 17.11. @jkoritzinsky @elinor-fung @eiriktsarpalis @tarekgh - thoughts on doing so for .NET 9.0?

jkoritzinsky commented 1 month ago

I believe we can do so. At this point, the most recent version release is 2024.11 Preview 4, so the package version we can update to today is technically 4.11.0-4.final if it existed, but since that version isn't available, 4.11.0-2.final can be used in its place.

jkoritzinsky commented 1 month ago

Once we've released the product, we can move to 4.11.0.

ericstj commented 1 month ago

We should do this in RC1 if possible so that we give it some time to iron out any issues.

ericstj commented 4 weeks ago

Seems like we should update the value of MicrosoftCodeAnalysisVersion_LatestVS

We should update Configuration and Options generators to use this property instead of MicrosoftCodeAnalysisVersion_4_8

I can try this, if it goes in smoothly we can take it, if not I might need some help.

ericstj commented 4 weeks ago

We actually pack this analyzer in the nuget package - so it cannot remove the lightup code or it would break folks using the package on .NET 8.0 / older VS. I think this is going to be a won't fix for 9.0. We'd need to multi-target the generator to remove this lightup code and that doesn't feel like the right tradeoff since it already does light-up.