Suppress EventSource trim warnings on BeginExecute and EndExecute - #4703
Open
charlesroddie wants to merge 1 commit into
Open
charlesroddie wants to merge 1 commit into
charlesroddie wants to merge 1 commit into
Conversation
These events have no typed EventSource.WriteEvent overload, so they use WriteEvent(int, object[]), which requires unreferenced code. Its message says the warning can be suppressed when the arguments are primitive, and these payloads are only int and string. WriteEventCore carries the same annotation, so rewriting them would still need the suppression. The Logging project targets netstandard2.0, which lacks UnconditionalSuppressMessageAttribute, so add an internal copy; the trimmer recognizes it by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The warning suppression lacks an automated trimming or Native AOT regression test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Suppresses two Native AOT trimming warnings, advancing issue #1947.
Changes:
- Adds a netstandard2.0 suppression-attribute polyfill.
- Suppresses IL2026 for primitive-only execution events.
File summaries
| File | Description |
|---|---|
UnconditionalSuppressMessageAttribute.cs |
Adds the trimmer-recognized polyfill. |
SqlClientEventSource.cs |
Suppresses warnings on execution events. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1810
to
+1811
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026", | ||
| Justification = "The payload is only int and string values, which EventSource writes without reflection.")] |
Author
There was a problem hiding this comment.
Trim warnings are being tested locally in order to make the reports in the descriptions. A small app could be checked in so there is confirmation in the repo but perhaps in a separate PR?
charlesroddie
marked this pull request as ready for review
September 16, 2026 20:16
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the work toward #1947.
Problem
SqlClientEventSource.BeginExecuteandEndExecuteare the only events without a typedEventSource.WriteEventoverload for their argument lists, so they callWriteEvent(int, params object[]). That overload is markedRequiresUnreferencedCode, so trimming or Native AOT publishing an application that uses SqlClient reports IL2026 for both.The library analyzer never shows these, because the Logging project targets netstandard2.0, whose reference assemblies don't carry the annotation. They appear only when an application is published.
Change
Suppress IL2026 on both methods. The runtime's message for the overload says the warning "can be suppressed if the object is a primitive type", and these payloads are only
intandstring. This is the same justification the runtime uses for its own typed overloads.Rewriting the methods with
WriteEventCorewould not avoid the suppression:WriteEventCorecarries the same annotation. It would also need unsafe code, and the payload encoding would need to stay exactly as it is for OpenTelemetry and Application Insights.netstandard2.0 has no
UnconditionalSuppressMessageAttribute, so the Logging project gets an internal copy, as dotnet/runtime does for its own netstandard2.0 builds. The trimmer recognizes the attribute by name. The Logging assembly grants internals access only to a test assembly that doesn't exist, so the copy can't clash with the public attribute anywhere.No change to the events' names, opcodes or payloads.
Testing
TrimmerSingleWarn=false, of a small ASP.NET Core application that opens a connection, runs a stored procedure and reads results: SqlClient's reachable warnings go from 42 to 40, the two removed being these.EventListeneronMicrosoft.Data.SqlClient.EventSourcereceives both events with the same names, opcodes, payload names and values, and no EventSource error events.EventSourceTestneeds a SQL Server instance and was not run.🤖 Generated with Claude Code