Add an EnableAppConfig switch to remove trim warnings - #4697
charlesroddie wants to merge 4 commits into
Conversation
On .NET, SqlClient reads app.config from three places: the configurable retry logic manager, LocalAppContextSwitches' own static constructor, and SqlAuthenticationProviderManager. ConfigurationManager.GetSection resolves section handler types named as strings in the config file, so reaching it from anywhere produces five trim warnings inside System.Configuration's TypeUtil that no annotation in SqlClient can remove. dotnet/runtime#49062 is closed with no fix planned. Gate all three readers behind a new EnableAppConfig switch, defaulting to true, and stub it through ILLink.Substitutions.xml as UseManagedNetworking already is. Publishing with the switch set to false removes those five warnings and the five in SqlConfigurableRetryLogicLoader, measured on a Native AOT application. The retry guards sit at the SqlCommand and SqlConnection call sites rather than inside SqlConfigurableRetryLogicManager, whose static field initializer would otherwise still run and keep the configuration reading reachable. 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. |
There was a problem hiding this comment.
🟡 Changes recommended
The disabled path introduces per-command and per-connection allocations and lacks behavioral coverage for the gated readers.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an opt-out switch for app.config loading to reduce trimming and Native AOT warnings while preserving existing behavior by default.
Changes:
- Adds and documents
EnableAppConfig. - Guards retry, authentication, and switch-override configuration reads.
- Adds linker substitutions and default-value test support.
File summaries
| File | Description |
|---|---|
.github/instructions/features.instructions.md |
Documents the switch. |
LocalAppContextSwitches.cs |
Defines and applies the switch. |
SqlAuthenticationProviderManager.cs |
Gates authentication configuration. |
SqlCommand.cs |
Gates command retry configuration. |
SqlConnection.cs |
Gates connection retry configuration. |
ILLink.Substitutions.xml |
Enables publish-time substitution. |
LocalAppContextSwitchesHelper.cs |
Adds test access to the switch. |
LocalAppContextSwitchesTest.cs |
Verifies the default value. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
With EnableAppConfig off, each command and connection created its own no-retry provider, where the manager shares one of each process-wide. Keep a lazily created shared provider for each instead, without touching SqlConfigurableRetryLogicManager. Add tests that the switch selects between the shared providers and the manager's. The authentication provider and switch override readers run in static constructors, so they cannot be tested in-process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On .NET Framework, LocalDbApi still read system.data.localdb from app.config with the switch off, so the switch did not mean the same thing on every platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation must distinguish runtime switch behavior from the publish-time configuration required for trimming.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
Also list the .NET Framework system.data.localdb section among what the switch gates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The switch consistently gates all identified configuration readers, preserves default behavior, and includes focused documentation and tests.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| // If configuration section is not yet found, try with old Configuration Section name for backwards compatibility | ||
| configurationSection = FetchConfigurationSection<SqlAuthenticationProviderConfigurationSection>(SqlAuthenticationProviderConfigurationSection.Name); | ||
| // New configuration section "SqlClientAuthenticationProviders" for Microsoft.Data.SqlClient accepted to avoid conflicts with older one. | ||
| configurationSection = FetchConfigurationSection<SqlClientAuthenticationProviderConfigurationSection>(SqlClientAuthenticationProviderConfigurationSection.Name); |
There was a problem hiding this comment.
This will remove the trim warning - does it allow ILLink to trim away every type within the System.Configuration assembly? We can use sizoscope to check this, it'll remove around 1.4MB if so.
There was a problem hiding this comment.
Not every type but most. System.Configuration.ConfigurationManager.dll goes from 443KB untrimmed, to 133KB trimmed before this PR (i.e. with the switch on), to 42KB trimmed with the switch off.
The residual is to do with SqlAuthenticationProviderManager where is ongoing work.
There was a problem hiding this comment.
Without loading data from app.config, the constructor for SqlAuthenticationProviderManager has no work to do. One way to remove this might be to have a private parameterless constructor which no-ops or logs; the class could be instantiated with that if the EnableAppConfig switch is disabled.
Part of the work toward #1947.
Problem
On .NET, SqlClient reads app.config from three places:
SqlConfigurableRetryLogicManager, for the retry sections. Reached fromSqlConnection.Openand everySqlCommand.Execute*, which readRetryLogicProviderunconditionally.LocalAppContextSwitches' static constructor, for theAppContextSwitchOverridessection.SqlAuthenticationProviderManager's static constructor, for the auth provider sections.ConfigurationManager.GetSectionresolves section handler types named as strings in the config file, so reaching it from anywhere leaves five trim warnings insideSystem.Configuration.TypeUtil(IL2026, two IL2057, IL2067, IL2070). They are in the dependency rather than in SqlClient, so no annotation here can remove them, and they are a fixed cost rather than a per-call-site one: a single ungated caller keeps all five. dotnet/runtime#49062 is closed with no fix planned, and upstream's guidance is to useMicrosoft.Extensions.Configurationinstead, which is what the@TODOcomments on bothFetchConfigurationSectioncopies already anticipate.Annotating public entry points with
RequiresUnreferencedCodeis not an option for the retry path, because it is reached fromOpenandExecute*rather than from retry-specific API, so the annotation would propagate onto every caller of those methods.Change
A new
Switch.Microsoft.Data.SqlClient.EnableAppConfig, defaulting totrue, with a guard at each app.config reader, including the .NET Frameworksystem.data.localdbone. When the switch is fixed at publish time,ILLink.Substitutions.xmlmakes the trimmer treat the property as a constant, as it already does forUseManagedNetworking, so the configuration reading becomes dead code and is removed.The retry guards sit at the
SqlCommandandSqlConnectioncall sites rather than insideSqlConfigurableRetryLogicManager, because that class builds its loader in a static field initializer, so touching the type at all would run the configuration reading regardless of the guard.Measurement
Native AOT publish of a small ASP.NET Core application that opens a connection, runs a stored procedure with a
DataTable-valued parameter inside a transaction, reads results, and uses Azure Monitor OpenTelemetry, withTrimmerSingleWarn=false. Measured on top of main plus #4683, #4684 and #4688, counting only warnings the application can reach:falseThe ten removed are the five in
System.Configuration.TypeUtiland the five inSqlConfigurableRetryLogicLoader.Behaviour
Default
truekeeps today's behaviour exactly. Set tofalse, app.config is ignored: no configurable retry logic from config, no config-declared authentication providers, noAppContextSwitchOverrides, and on .NET Framework nosystem.data.localdbinstances. This is a real behaviour change rather than a pure trimming knob, so it is opt-in per application.The switch is read before any override is applied, so it cannot itself be set from the app.config that it gates. It has to be set through
AppContextorruntimeconfig.Open question
features.instructions.mdsays new switches should default tofalseand opt in to new behaviour. This one gates behaviour that already exists, and the default has to mean "the feature is present" for the substitution to trim the disabled path, so it defaults totrue. Happy to rename it toDisableAppConfigif you would rather follow the guideline literally, though that makes the substitution entries read backwards.🤖 Generated with Claude Code