You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
ILLink analyzer and code fixer passed twice to the csc #84119
This looks like it's a consequence of referencing ILLink.Tasks as a package (dotnet/sdk#29441) and moving the analyzers into the ILLink.Tasks package (#79609), while also continuing to bundle analyzers with the SDK (changed to pick up dlls from the ILLink.Tasks package in dotnet/sdk#30256).
@tlakollo I believe we discussed including analyzers with ILLink.Tasks but I don't remember the context - do you remember what motivated the change? cc @ViktorHofer@agocke I think it in general makes sense for the analyzer version to match the illink version.
Summarizing some discussion today about whether the trim analyzer should version with the TFM or with the SDK:
In the past, when we've made breaking changes (changed warning codes, or fix analysis holes), we had to find hacky ways for the new analyzer not to warn on old TFMs. Versioning with the TFM is more backwards compatible and won't have this problem. The .NET platform analyzers version this way. There is some risk that using an older analyzer with a newer roslyn could break or produce spurious warnings if there are breaking changes in the model exposed by roslyn to analyzers (for example, representing existing language structures with new node types).
On the other hand, older versions of the analyzer might not support newer language features, and typically produce spurious warnings when encountering new language features. Using a recent (SDK-versioned) analyzer would not have this problem. Roslyn originally expected analyzers to work this way. This path is less likely to cause problems if the analyzer/roslyn interface changes, because more recent analyzers will have a chance to react to roslyn changes.
We were leaning towards versioning with the TFM, mainly because we aren't confident that we won't have to make some kind of breaking change to the linker/analyzer semantics, and we don't have a great solution for preventing warnings on old TFMs with the latest analyzer.
We were leaning towards versioning with the TFM, mainly because we aren't confident that we won't have to make some kind of breaking change to the linker/analyzer semantics, and we don't have a great solution for preventing warnings on old TFMs with the latest analyzer.
Also pointing out that code is shared between the analyzer, linker, and AOT compiler -- so sharing the same release cadence and bug bar among all three makes sense.
Activity
vitek-karas commentedon Mar 30, 2023
@sbomer - could you please have a look what's going on?
sbomer commentedon Mar 30, 2023
This looks like it's a consequence of referencing ILLink.Tasks as a package (dotnet/sdk#29441) and moving the analyzers into the ILLink.Tasks package (#79609), while also continuing to bundle analyzers with the SDK (changed to pick up dlls from the ILLink.Tasks package in dotnet/sdk#30256).
@tlakollo I believe we discussed including analyzers with ILLink.Tasks but I don't remember the context - do you remember what motivated the change? cc @ViktorHofer @agocke I think it in general makes sense for the analyzer version to match the illink version.
For reference, some of the other analyzers (like Microsoft.CodeAnalysis.NetAnalyzers) ship with the SDK (https://github.com/dotnet/sdk/blob/ec824c9797659318c772330cb176286643a25dfe/src/Layout/redist/targets/GenerateLayout.targets#L54), but have logic to prevent duplicate analyzers if you reference the package explicitly (https://github.com/dotnet/roslyn-analyzers/blob/9a0d72e1eba39b8681aba054c9095069fb54314b/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs#L271C31-L272).
For us, one possible fix is to stop bundling analyzers with the SDK, and rely on the packagereference mechanism. That should prevent duplicate warnings even if someone explicitly references ILLink.Tasks. We'll need to adjust this condition to ensure the ILLink.Tasks package gets restored in any scenario where the analyzer is enabled: https://github.com/dotnet/sdk/blob/ec824c9797659318c772330cb176286643a25dfe/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L20.
sbomer commentedon Apr 19, 2023
Summarizing some discussion today about whether the trim analyzer should version with the TFM or with the SDK:
In the past, when we've made breaking changes (changed warning codes, or fix analysis holes), we had to find hacky ways for the new analyzer not to warn on old TFMs. Versioning with the TFM is more backwards compatible and won't have this problem. The .NET platform analyzers version this way. There is some risk that using an older analyzer with a newer roslyn could break or produce spurious warnings if there are breaking changes in the model exposed by roslyn to analyzers (for example, representing existing language structures with new node types).
On the other hand, older versions of the analyzer might not support newer language features, and typically produce spurious warnings when encountering new language features. Using a recent (SDK-versioned) analyzer would not have this problem. Roslyn originally expected analyzers to work this way. This path is less likely to cause problems if the analyzer/roslyn interface changes, because more recent analyzers will have a chance to react to roslyn changes.
We were leaning towards versioning with the TFM, mainly because we aren't confident that we won't have to make some kind of breaking change to the linker/analyzer semantics, and we don't have a great solution for preventing warnings on old TFMs with the latest analyzer.
@vitek-karas @agocke
agocke commentedon Apr 19, 2023
Also pointing out that code is shared between the analyzer, linker, and AOT compiler -- so sharing the same release cadence and bug bar among all three makes sense.
Reference ILLink analyzer matching ILLink version (#32045)