Switch to built-in IsTrimmable property#41021
Conversation
JamesNK
added
the
area-infrastructure
JamesNK
requested review from
a team,
BrennanConroy,
Pilchie,
Tratcher,
dougbu,
halter73 and
wtgodbe
as code owners
|
Trimming analyzers are finding new errors and breaking a lot. |
|
A new issue for fixing the NullReferenceException and NotImplementedException errors in analyzer - dotnet/linker#2718 The analyzer is finding errors that ILLink didn't report. Is that expected? I thought ILLinker was the authority but it seems like analyzers and the linker each contribute different warnings. |
|
We've been trying to match the warnings provided by the linker in the analyzer, so that's likely why you're hitting new issues. The analyzer is undergoing significant development, so these are new bugs that are introduced. If you file bugs we should fix them ASAP. The linker is basically the authority, and any bug where the analyzer is different is likely a bug in the analyzer -- with a few minimal exceptions around things that can't be properly represented in metadata. Thanks for turning this on, we can look through and investigate the problems. |
|
It will take time for analyzer fixes to be made and the dependency to flow to aspnetcore. I'll keep IsTrimmable but update this PR to disable analyzers. Can reenable once they're fixed. |
| </PropertyGroup> | ||
|
|
||
| <!-- Trim analyzers are automatically enabled by IsTrimming=true. They currently break when run on aspnetcore. | ||
| Remove when trimming analyzers are fixed: https://github.com/dotnet/aspnetcore/issues/41016 --> |
There was a problem hiding this comment.
Would it make more sense to link to the linker issue? That way people finding this comment can go directly to the linker issue and see if it is fixed yet or not.
There was a problem hiding this comment.
I'm going to leave the aspnetcore issue open with a link to the linker issue.
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
LGTM. Just one minor nit about the comment.
| <!-- Trim analyzers are automatically enabled by IsTrimming=true. They currently break when run on aspnetcore. | ||
| Remove when trimming analyzers are fixed: https://github.com/dotnet/aspnetcore/issues/41016 --> | ||
| <PropertyGroup> | ||
| <EnableTrimAnalyzer Condition=" '$(EnableTrimAnalyzer)' == ''">false</EnableTrimAnalyzer> |
There was a problem hiding this comment.
I don't see this set anywhere else. Why include the Condition❔
There was a problem hiding this comment.
Just in case we want to enable for some projects later to test whether it works or not.
There was a problem hiding this comment.
/fyi the Condition doesn't help that case much because projects are read after Directory.Build.props files and (therefore) this file. The project can therefore override any property it likes (as long as it doesn't use a != '' Condition of course).
But my comment was definitely a nit.
ghost
added this to the
4 participants
Fixes #41016