Skip to content

Copy MemberNotNull(When) and the trim/AOT attributes; fix resolver leak into the host runtime - #5

Open
christophwille wants to merge 1 commit into
masterfrom
copy-membernotnull-aot-attributes
Open

christophwille wants to merge 1 commit into
masterfrom
copy-membernotnull-aot-attributes

Conversation

@christophwille

Copy link
Copy Markdown
Member

Summary

Addresses follow-up 6 of #1: the weaver only copied the C# 8 nullable attributes. It now also copies:

Attribute Uses in .NET 10 refs Consumer effect
MemberNotNullWhen 2 (Task.IsFaulted) Removes a false CS8602 (see below)
MemberNotNull 0 None today; same small file, ready for newer refs
RequiresUnreferencedCode 812 Only for tools outside the SDK (see Decisions)
RequiresDynamicCode 448 same
DynamicallyAccessedMembers (+ DynamicallyAccessedMemberTypes enum) 426 same
RequiresAssemblyFiles 15 same

Counts come from scanning Microsoft.NETCore.App.Ref 10.0.14 ref/net10.0 with System.Reflection.Metadata.

When the target framework doesn't define one of these types, it is created as an internal type in the output assembly, the same way as the existing nullable attributes. The provided types are ported from tunnelvisionlabs#95 with the namespace changed.

While testing, the new attributes exposed two existing weaver bugs, both fixed here (see Bugs fixed).

The Task.IsFaulted false positive

Task.IsFaulted carries [MemberNotNullWhen(true, nameof(Exception))], and the API exists on netstandard and .NET Framework. The annotator made Task.Exception nullable (AggregateException?) but dropped that guarantee. So against annotated references this warned:

task.IsFaulted ? task.Exception.Message : ""   // CS8602 before, clean now

Verified red/green with tests/SingleTFM (netstandard2.0): CS8602 with the previous package, clean with this one. The compiler honors the attribute from metadata at LangVersion 8, which the test projects use.

Decisions and alternatives

1. Trim/AOT attributes: copied, although the SDK ignores them for these TFMs

I measured this with a probe project: the .NET SDK never runs the trim/AOT analyzers on netstandard2.0 or net48. With IsTrimmable/IsAotCompatible, or even with EnableTrimAnalyzer=true forced, it emits NETSDK1210/NETSDK1212, and the ILLink analyzer is not added. An override carrying [RequiresUnreferencedCode] whose base method lacks it reports no IL2046 on netstandard2.0, but does on net8.0. The annotator only produces .NET Framework, .NET Standard and .NETCoreApp < 3.0 references, so these attributes have no effect in normal SDK builds.

  • Option A: MemberNotNull(When) only. Pro: everything copied has a measurable effect, and it is less code. Con: references stay incomplete for third-party tools that read these attributes.
  • Option B (taken): also copy the trim/AOT attributes. Pro: annotated references match .NET 10 more closely and are ready if tooling ever supports these TFMs. Con: about 5 more files plus an enum. No SDK-based test is possible, so only a unit test that reads the output back with Cecil covers them.

2. Not copied: DynamicDependency, UnconditionalSuppressMessage

Both have 0 uses in the .NET 10 reference assemblies, because they only appear in implementation code. Skipping them also avoids tunnelvisionlabs#95's Type-typed constructor arguments, which would need importing across modules.

3. Named arguments are still skipped

Annotate skips any attribute that has named arguments. tunnelvisionlabs#95 added code to copy them but kept that skip, so its copy code never ran. Only 3 of 812 RequiresUnreferencedCode uses have one (Url = ...), and those 3 stay uncopied.

  • Pro of keeping the skip: no new code path for the type of a named argument.
  • Con: those 3 attributes are lost. Copying them, or dropping only Url, is a small follow-up if those APIs matter.

4. PolySharp: not used

As in #1, PolySharp would replace the shipped NullableAttributes.cs/.vb, which changes what every consumer compiles. Nothing here needs it.

5. NullableAttributes.cs/.vb unchanged

Adding MemberNotNull(When) to the source file shipped to consumers would let netstandard/netfx consumers use these attributes in their own code.

  • Con: any consumer that already defines these attributes by hand would break with CS0101. netstandard2.1/netcoreapp3.x consumers don't get the file at all (_FrameworkIncludesNullableAttributes), so they would need their own condition.
  • Left for a separate change.

6. Known behavior that this extends

Every attribute of interest is created in every annotated assembly, whether used or not (AddAttributeOfInterest resolves the type up front). This adds 6 more internal types per annotated assembly, the same as for the nullable attributes today.

Bugs fixed

  1. AssemblyResolver leaked into the build host's runtime (dotnet MSBuild only). Dependencies were loaded with ReaderParameters that had no AssemblyResolver. Cecil's default resolver, which on .NET Core searches the running runtime, then resolved their references. For netcoreapp2.x, Microsoft.CSharp.dll references netstandard. The facade's forward of System.Object was resolved in the host's shared runtime, so the weaver used the host's public System.Private.CoreLib attribute types instead of creating its own. The old attributes survived this by accident. With the new ones, the build crashed (MSB4018, Sequence contains more than one matching element) on netcoreapp2.0/2.1, MultiTFM and NoDisabledWarnings. msbuild.exe (the net472 task) was not affected. The resolver now passes itself to the assemblies it loads.
    • Side effect: under dotnet, outputs for affected references now use their own internal attribute types instead of references into the host runtime's CoreLib. That is what msbuild.exe already did.
  2. The attribute constructor lookup matched property setters. IsMatchingConstructor didn't check IsConstructor, so a real attribute type's set_Url(string) matched the (string) constructor. It now only considers constructors. This was the immediate cause of the crash in 1.

Tests

  • Integration: tests/{SingleTFM,MultiTFM,NoDisabledWarnings}/Class1.cs and tests/WPF/UserControl1.xaml.cs get the Task.IsFaulted line under #if !NET35 && !NETCOREAPP3_0 && !NETCOREAPP3_1. net35 has no Task; netcoreapp3.x isn't annotated and its own references lack the attribute. It is not added to RefVersion3.1.0, because the 3.1 references lack the attribute too.
  • ProgramMainTests (new): runs Program.Main end to end on netstandard2.0's netstandard.dll (from a NETStandard.Library 2.0.3 PackageDownload), annotated from the net10.0 netstandard.dll the test project compiles against. The Microsoft.NETCore.App.Ref download stays removed there because of NU1505. It reads the output back and checks:
    • Task.IsFaulted gets MemberNotNullWhen(true, "Exception")
    • Type.GetMethods(BindingFlags) gets DynamicallyAccessedMembers with value 24 (PublicMethods | NonPublicMethods), which covers the enum argument end to end
    • Assembly.GetTypes / Type.MakeGenericType / Assembly.GetFiles get RequiresUnreferencedCode / RequiresDynamicCode / RequiresAssemblyFiles
    • all created types are internal
  • Resolver regression test (AssemblyResolver_LoadsDependenciesWithItself): resolving Object through the System.Runtime facade must end in the search directory. It fails without the fix and passes with it.

This is also the first end-to-end test of the weaver that doesn't need a full MSBuild run.

Verification

  • dotnet build ReferenceAssemblyAnnotator.slnx -c Release: clean (warnings as errors, StyleCop)
  • dotnet test --project ICSharpCode.ReferenceAssemblyAnnotator.Tests -c Release --no-build: 25/25
  • ./build/test.ps1 -Configuration Release -Engine dotnet: 17/17
  • ./build/test.ps1 -Configuration Release -Engine msbuild: 17/17

🤖 Generated with Claude Code

Copy MemberNotNull, MemberNotNullWhen, RequiresUnreferencedCode,
RequiresDynamicCode, RequiresAssemblyFiles and DynamicallyAccessedMembers
from the annotated reference assemblies. Missing attribute types (and the
DynamicallyAccessedMemberTypes enum) are created as internal types, as
for the existing nullable attributes.

Task.IsFaulted carries [MemberNotNullWhen(true, nameof(Exception))];
without it, annotated references made `task.IsFaulted ?
task.Exception.Message : ...` warn CS8602.

Also fix two existing bugs that the new attributes exposed:
- AssemblyResolver loaded dependencies without itself as their resolver,
  so Cecil's default resolver found the build host's runtime (dotnet
  MSBuild only). For netcoreapp2.x references the weaver then used the
  host's System.Private.CoreLib attribute types.
- The attribute constructor lookup also matched property setters such as
  set_Url(string).

Tests: a Task.IsFaulted line in the integration test sources, and
ProgramMainTests, which runs Program.Main end to end on netstandard2.0's
netstandard.dll and checks the resolver.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant