Repository navigation
Conversation
|
Hi Jeremy, you should have asked before embarking on this endeavor. Because ics.d is intentionally netstandard only and will remain netstandard for the forseeeable future, and multi-targetting is absolutely off-limits (we had that discussion internally more than once). Reason why no multi-targetting: then all our projects (tests, ILSpy) would start using the net10 tfm compiled variant and basically the netstandard target would go untested. And netstandard is our ticket for being included in Roslyn. If you can come up with a different approach from multi-targetting, we can talk. |
Is my latest commit an acceptable solution to this concern? |
|
@siegfriedpammer Would having the SetTargetFramework attribute on icsd.tests suffice, and the rest of ILSpy use the net10 target? That way both TFMs should be properly exercised in our repository. |
| </GetPackageVersionDependsOn> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))"> |
There was a problem hiding this comment.
If we target net10, why would we compare to net80?
There was a problem hiding this comment.
That's the earliest runtime that IsAotCompatible is valid. A warning is issued if the property is applied on an earlier framework.
NETSDK1210
IsAotCompatible and EnableAotAnalyzer are not supported for the target framework. Consider multi-targeting to a supported framework to enable ahead-of-time compilation analysis, and set IsAotCompatible only for the supported frameworks. For example:
<IsAotCompatible Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">true</IsAotCompatible>
|
Note This comment was entirely AI generated (Claude Fable 5 via Claude Code) on behalf of @christophwille. Here is an assessment of the TL;DR
Alternatives, ranked for the constraint1. Stay netstandard2.0-only and polyfill the annotation attributes (recommended)The trimmer/ILC match Result: one TFM, one binary, the tested binary is the shipped binary, nothing about the Roslyn-inclusion story changes, and none of the csproj churn in consumers is needed. Most of the genuinely valuable content of this PR — the actual AOT code fixes, obsoletions, and suppressions with justifications — carries over unchanged. The one real gap: the SDK's trim/AOT analyzers don't run on a netstandard2.0 TFM, so annotations can't be verified at build time. That can be fixed with a non-packed validation build — either a CI-only sibling csproj compiling the same sources under net10.0 with 2. If shipping two TFMs is acceptable: centralize the pin and test bothInstead of ~12 scattered <ProjectReference Update="@(ProjectReference)"
Condition="'%(FileName)' == 'ICSharpCode.Decompiler'"
SetTargetFramework="TargetFramework=netstandard2.0" />in the root The NuGet-restore caveat above still applies and needs an 3. Conditional TFM (net10 only when packing) — reject
Bottom lineIf "netstandard is the product and must be the tested thing" is a hard rule, then multi-targeting solves the problem in the wrong place. Option 1 gives downstream NativeAOT consumers exactly what this PR set out to deliver (no AOT warnings, real suppressions with justifications, the Suggested direction: keep the code fixes and suppressions, replace the multi-target + |
| string runtimeDir = Path.GetDirectoryName(typeof(object).Module.FullyQualifiedName)!; | ||
| string? runtimeDir = Path.GetDirectoryName(GetSystemObjectModuleFullyQualifiedName()); | ||
| if (string.IsNullOrEmpty(runtimeDir)) | ||
| goto NotFound; |
There was a problem hiding this comment.
Why not use a static local function?
|
@ds5678 would polyfilling the attributes work for you? |
AOT warnings only appear on versions of .NET that have been annotated, which was the original reason for multi-targeting .NET 10. Without the compiler issuing warnings, code changes can silently break AOT compatibility. |
|
But Claude also suggested "helper" constructs to verify the warnings to the side. |
|
I assume that this is the ideal solution? |
|
Isn't ReferenceAssemblyAnnotator pretty much dead? |
|
@ds5678 Did you have a chance to look into option 1 (without touching ReferenceAssemblyAnnotator). We do want to support your AOT needs. |
I was waiting to see if there was any movement on my ReferenceAssemblyAnnotator pull request because I think that is the best solution. If another week goes by with no news, I'll work on the option you suggested. |
|
#4241 adds a few AOT fixes |
* Polyfill UnconditionalSuppressMessageAttribute for netstandard2.0 ILLink and ILC match the trim annotation attributes by namespace and name, so an internal copy in the netstandard2.0 build is honored exactly like the framework's own type on .NET 5 and later. This keeps the single target framework, which was the maintainers' condition when the multi-targeting approach of #3946 was rejected, and avoids making the compiled IL depend on attribute types that .NET Framework hosts (the VS add-in, Windows PowerShell) cannot resolve. The guard on both polyfill files keeps a .NETCoreApp compile of these sources free of duplicates. Assisted-by: Claude:claude-fable-5-1:Claude Code * Keep decompiler frames in DecompilerException traces under Native AOT StackFrame.GetMethod() is annotated RequiresUnreferencedCode and returns null for every frame without reflection metadata, which under Native AOT is most of them. The custom stack trace format skipped such frames, so a DecompilerException from a native host reached the issue tracker with the decompiler's own frames missing; the runtime does know the method names and prints them through StackFrame.ToString(). Printing that description for method-less frames keeps the frames, and the IL2026 suppression states the handled case rather than merely the null check. Assisted-by: Claude:claude-fable-5-1:Claude Code * Suppress IL2026 on the four untyped DecompilerEventSource events netstandard2.0 has no typed WriteEvent overload for these argument shapes, so they bind to WriteEvent(int, params object[]), whose RequiresUnreferencedCode annotation warns about serializing arbitrary object graphs and names primitives and strings as the safe case. These events carry nothing else, so suppressing per event is the right size; rewriting them with WriteEventCore would add unsafe code for no change in behaviour. Assisted-by: Claude:claude-fable-5-1:Claude Code * Resolve framework facades on single-file and Native AOT hosts Such hosts run from their application directory, which holds no managed assemblies, so everything the resolver took from the directory of the running runtime (the mscorlib and netstandard facades for .NET Framework and netstandard inputs on hosts without a Windows framework directory, the last-resort probe, the ref-pack documentation) came up empty there. The newest shared framework of the dotnet on PATH serves the same purpose; a managed host keeps using its own runtime directory, which is that shared framework already. Measured with the AOT gate on Linux: a .NET Framework assembly decompiled identically by the native and the managed build only after this change. The same run showed that with ThrowOnAssemblyResolveErrors set, the corlib lookup threw NotSupportedException for every .NET Framework reference on such hosts although later steps of the chain (the shared framework among them) could still resolve it; those lookups now yield null and leave the ResolutionException to the end of the chain. Assisted-by: Claude:claude-fable-5-1:Claude Code * Mark ICSharpCode.Decompiler as trimmable and AOT-compatible The SDK writes these two AssemblyMetadata attributes from the IsTrimmable and IsAotCompatible properties for .NETCoreApp targets only; on netstandard2.0 the properties produce NETSDK1212 and NETSDK1210 and nothing else, so the items are declared by hand. ILLink and ILC read the IsTrimmable marker: warnings from the assembly are then reported per call site instead of as one collapsed IL2104, and TrimMode=partial consumers trim it instead of rooting it whole. The analyzers' VerifyReference*Compatibility checks read both markers. This lands after the warning sites were fixed on purpose, because the marker exposes any remaining warning to every consumer. Assisted-by: Claude:claude-fable-5-1:Claude Code * Add the Native AOT gate for the ICSharpCode.Decompiler package The trim and AOT analyzers cannot run on a netstandard2.0 project, so nothing in the build would notice a new reflection pattern in the decompiler until a consumer's publish warned about it. Publishing a rooted PublishAot app with warnings as errors checks the shipped binary with the tool consumers use, including compiler-generated code and the polyfilled attributes, which a net10 analyzer compile would never exercise. Running the native binary and the managed build on the same inputs catches behaviour that differs at runtime rather than in the analysis, as the stack trace frames and the framework facade resolution did. The project stays out of the solution because PublishAot pulls the ILCompiler packs into every restore. Assisted-by: Claude:claude-fable-5-1:Claude Code * Document trimming and Native AOT behaviour in the package readme Consumers hosting the decompiler in a PublishAot app have no way to tell from the package that the tracing provider is off by default there, or that framework facades come from the dotnet on PATH on single-file and Native AOT hosts. Assisted-by: Claude:claude-fable-5-1:Claude Code * Check the MoveNext handle for IsNil instead of comparing it to null A MethodDefinitionHandle is a struct, so the null comparison was always false and the check never did anything; CreateILAst rejects the nil handle later, which is why the fallback still worked. The explicit check says what was meant. Assisted-by: Claude:claude-fable-5-1:Claude Code * Drop the NET5_0_OR_GREATER guard from the attribute polyfills These sources only ever compile as netstandard2.0 (and as net472 in the VS add-in), so the condition was always true; it only existed for a .NETCoreApp analyzer compile of the same sources that is not part of the repository. Assisted-by: Claude:claude-fable-5-1:Claude Code * Say where the AOT gate runs instead of calling it a leg The gate has a single CI job, on Linux; "the Linux leg" suggested a Windows counterpart that does not exist. The comment now names the job and why Linux is the host that matters. Assisted-by: Claude:claude-fable-5-1:Claude Code
|
@ds5678 closing this one as the other two PRs have been merged. It would be great if you could give our changes a spin to see if we caught everything! |
ILLink and ILC match the trim annotation attributes by namespace and name, so an internal copy in the netstandard2.0 build is honored exactly like the framework's own type on .NET 5 and later. This keeps the single target framework, which was the maintainers' condition when the multi-targeting approach of icsharpcode#3946 was rejected, and avoids making the compiled IL depend on attribute types that .NET Framework hosts (the VS add-in, Windows PowerShell) cannot resolve. The guard on both polyfill files keeps a .NETCoreApp compile of these sources free of duplicates. Assisted-by: Claude:claude-fable-5-1:Claude Code
Yes, I'm no longer getting build warnings on my end. Thank you for working on this! |
3 participants
Problem
Currently, the
ICSharpCode.Decompilerpackage is not annotated for AOT compatibility, so downstream consumers get build warnings when trying to compile with NativeAOT.Solution
UnconditionalSuppressMessageattributes to suppress build warnings where necessary. Of course, all suppressions are annotated with justifications for why they're actually safe.UnconditionalSuppressMessageis only available on .NET 5 and later.