Treat old TFMs as unsupported by ILLink analyzers#35837
Merged
sbomer merged 4 commits intodotnet:mainfrom Oct 5, 2023
Merged
Conversation
Up to .NET 7, the analyzers versioned with the SDK (so you would always get the latest analyzers). This allowed using the analyzers (and related options which enable the analyzers) even on older TFMs. dotnet#32045 changed to versioning the analyzers with the target framework, but still allowed use of the analyzers on target frameworks where the .NET 7 ILLink pack was made available for backwards compatibility. This change removes such backwards compat by treating the analyzers as unsupported for target frameworks before the framework was annotated with the relevant attributes. PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding analyzers. With this change, we now need to avoid failing PublishTrimmed/PublishSingleFile for older TFMs that supported these publish options, but don't support the analyzers.
ghost
added
The breaking change shows up in this testcase, which was designed to demonstrate how one could work around the ProcessFrameworkReferences logic to reference the latest version of the analyzer in a netstandard project. The workaround now requires an additional setting.
dsplaisted
approved these changes
Oct 5, 2023
Comment on lines
+26
to
+32
| <_FirstTargetFrameworkToSupportTrimming>net6.0</_FirstTargetFrameworkToSupportTrimming> | ||
| <_FirstTargetFrameworkToSupportAot>net7.0</_FirstTargetFrameworkToSupportAot> | ||
| <_FirstTargetFrameworkToSupportSingleFile>net6.0</_FirstTargetFrameworkToSupportSingleFile> | ||
|
|
||
| <_FirstTargetFrameworkVersionToSupportTrimAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportTrimming)'))</_FirstTargetFrameworkVersionToSupportTrimAnalyzer> | ||
| <_FirstTargetFrameworkVersionToSupportAotAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportAot)'))</_FirstTargetFrameworkVersionToSupportAotAnalyzer> | ||
| <_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportSingleFile)'))</_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer> |
Member
There was a problem hiding this comment.
It doesn't look like the first set of properties are used anywhere else. Can we specify the version numbers directly instead of specifying the target frameworks and then parsing out the versions?
Suggested change
| <_FirstTargetFrameworkToSupportTrimming>net6.0</_FirstTargetFrameworkToSupportTrimming> | |
| <_FirstTargetFrameworkToSupportAot>net7.0</_FirstTargetFrameworkToSupportAot> | |
| <_FirstTargetFrameworkToSupportSingleFile>net6.0</_FirstTargetFrameworkToSupportSingleFile> | |
| <_FirstTargetFrameworkVersionToSupportTrimAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportTrimming)'))</_FirstTargetFrameworkVersionToSupportTrimAnalyzer> | |
| <_FirstTargetFrameworkVersionToSupportAotAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportAot)'))</_FirstTargetFrameworkVersionToSupportAotAnalyzer> | |
| <_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer>$([MSBuild]::GetTargetFrameworkVersion('$(_FirstTargetFrameworkToSupportSingleFile)'))</_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer> | |
| <_FirstTargetFrameworkVersionToSupportTrimAnalyzer>6.0</_FirstTargetFrameworkVersionToSupportTrimAnalyzer> | |
| <_FirstTargetFrameworkVersionToSupportAotAnalyzer>7.0</_FirstTargetFrameworkVersionToSupportAotAnalyzer> | |
| <_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer>6.0</_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer> |
Member
Author
There was a problem hiding this comment.
They're used by the IsTargetFrameworkCompatible checks added in #35767.
vitek-karas
approved these changes
Oct 5, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Up to .NET 7, the analyzers versioned with the SDK (so you would always
get the latest analyzers). This allowed using the analyzers (and related
options which enable the analyzers) even on older TFMs.
#32045 changed to versioning the
analyzers with the target framework, but still allowed use of the analyzers
on target frameworks where the .NET 7 ILLink pack was made available
for backwards compatibility.
This change removes such backwards compat by treating the analyzers
as unsupported for target frameworks before the framework was annotated
with the relevant attributes. This increases the scope of the warning added in
#29441 and #34077 to also warn for these unsupported
TFMs.
The first commit adds testcases to show the old behavior. The second commit
implements the new behavior and shows the changes to these testcases.
PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding
analyzers. With this change, we now need to avoid failing
PublishTrimmed/PublishSingleFile for older TFMs that supported these publish
options, but don't support the analyzers. This change handles that by only
defaulting the
Enable*Analyzersettings based on the publish settings forTFMs where the analyzers are known to be supported.