Skip to content

fix: correct the expression rewriter's trimming annotation and the analyzer package assets - #88

Merged
glennawatson merged 2 commits into
mainfrom
fix/trimming-annotations-and-analyzer-assets
Sep 9, 2026
Merged

fix: correct the expression rewriter's trimming annotation and the analyzer package assets#88
glennawatson merged 2 commits into
mainfrom
fix/trimming-annotations-and-analyzer-assets

Conversation

@glennawatson

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Two bug fixes.

What is the new behavior?

A trimmed consumer no longer sees IL2046 from the expression rewriter.

  • RequiresUnreferencedCode sits on ExpressionRewriter itself rather than on its VisitBinary,
    VisitUnary and VisitMethodCall overrides. An override cannot carry an annotation its base member lacks,
    and ExpressionVisitor carries none, so each of the three was reported.
  • The shared rewriter is built on first use instead of in a static initializer. A static constructor has
    nowhere to carry the annotation the type now needs, so Reflection.Rewrite states it; all three callers
    already reach it from a path annotated the same way.

Development-only analyzers are no longer package dependencies.

  • Three analyzer references gained PrivateAssets="all", matching the five beside them that already had it.

What is the current behavior?

Publishing a trimmed application that reaches the expression engine reports IL2046 three times, once per
annotated override.

Every packed project lists three repository-only analyzer packages as dependencies, so consumers inherit
build-time tooling in their dependency graph. The report named the generator package, but
src/Directory.Build.props applies to all of them, so the runtime and platform packages leak them too.

Closes #84
Closes #82

What might this PR break?

Reflection.Rewrite is public and now carries RequiresUnreferencedCode. A consumer calling it directly
from a trimming-annotated context will see IL2026 where it previously saw nothing - the warning is accurate,
since the method has always used reflection. Callers inside this repository are unaffected: all three already
sit on annotated paths.

Checklist

  • I have read the Contribute guide
  • Tests have been added or updated (for bug fixes / features)
  • Docs have been added or updated (for bug fixes / features)
  • Changes target the main branch
  • PR title follows Conventional Commits

Additional information

Both fixes were verified against the symptom rather than the source.

The trimming fix was reproduced first: a console app referencing the runtime library, reaching the expression
engine, published with PublishTrimmed. It reported IL2046 three times before the change and none after. The
two IL2026 warnings that remain are the app's own call to WhenAnyDynamic, which is deliberately
RequiresUnreferencedCode.

The packaging fix was checked by packing ReactiveUI.Binding and reading the dependency groups out of the
generated nuspec, which now list only ReactiveUI.Primitives and Splat.

Neither issue was caught by the AOT validation harness, because nothing in it reaches the expression engine.
Closing that gap needs a target that deliberately does, which cannot assert zero warnings the way the current
harness does - WhenAnyDynamic is annotated by design. Worth deciding separately.

…verrides

- RequiresUnreferencedCode on VisitBinary, VisitUnary and VisitMethodCall
  contradicts the ExpressionVisitor members they override, which a consumer's
  trimmed publish reports as IL2046 three times. The annotation belongs on the
  rewriter itself, where no base member disagrees with it.
- Build the shared rewriter on first use rather than in a static initializer:
  a static constructor has nowhere to carry the annotation the type now needs,
  so Reflection.Rewrite states it instead. Every caller already reaches it from
  a path that says the same thing.

Closes #84
- Three analyzer references carried no PrivateAssets, so they were published as
  dependencies of every packed project rather than staying a build-time concern.
- Affects the runtime and platform packages too, not only the generator package.

Closes #82
@sonarqubecloud

sonarqubecloud Bot commented Sep 9, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (4adab24) to head (da441b2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   99.48%   99.48%   -0.01%     
==========================================
  Files         246      246              
  Lines       10411    10410       -1     
  Branches     1124     1125       +1     
==========================================
- Hits        10357    10356       -1     
  Misses         42       42              
  Partials       12       12              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@glennawatson
glennawatson merged commit 5393d32 into main Sep 9, 2026
12 checks passed
@glennawatson
glennawatson deleted the fix/trimming-annotations-and-analyzer-assets branch September 9, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant