Add an EnableAppConfig switch to remove trim warnings

[charlesroddie]

Part of the work toward #1947.

Problem

On .NET, SqlClient reads app.config from three places:

• SqlConfigurableRetryLogicManager, for the retry sections. Reached from SqlConnection.Open and every SqlCommand.Execute*, which read RetryLogicProvider unconditionally.
• LocalAppContextSwitches' static constructor, for the AppContextSwitchOverrides section.
• SqlAuthenticationProviderManager's static constructor, for the auth provider sections.

ConfigurationManager.GetSection resolves section handler types named as strings in the config file, so reaching it from anywhere leaves five trim warnings inside System.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 use Microsoft.Extensions.Configuration instead, which is what the @TODO comments on both FetchConfigurationSection copies already anticipate.

Annotating public entry points with RequiresUnreferencedCode is not an option for the retry path, because it is reached from Open and Execute* 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 to true, with a guard at each app.config reader. It is .NET only: trimming does not apply to .NET Framework, where the switch is a constant true. When the switch is fixed at publish time, ILLink.Substitutions.xml makes the trimmer treat the property as a constant, as it already does for UseManagedNetworking, so the configuration reading becomes dead code and is removed.

The retry guards sit at the SqlCommand and SqlConnection call sites rather than inside SqlConfigurableRetryLogicManager, 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, with TrimmerSingleWarn=false. Measured on top of main plus #4683, #4684 and #4688, counting only warnings the application can reach:

	SqlClient	System.Configuration	total
Before	24	5	29
Publishing with the switch set to false	19	0	19

The ten removed are the five in System.Configuration.TypeUtil and the five in SqlConfigurableRetryLogicLoader.

Behaviour

Default true keeps today's behaviour exactly. Set to false, app.config is ignored: no configurable retry logic from config, no config-declared authentication providers, and no AppContextSwitchOverrides. 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 AppContext or runtimeconfig.

Open question

features.instructions.md says new switches should default to false and 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 to true. Happy to rename it to DisableAppConfig if you would rather follow the guideline literally, though that makes the substitution entries read backwards.

🤖 Generated with Claude Code

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 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.

[Copilot]

🟡 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

[Copilot]

🟢 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]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[edwardneal]

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.

[charlesroddie]

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.

[edwardneal]

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.

[charlesroddie]

That's a useful observation but leaving this out of this PR as the switch currenly does what it says literally and there are other PRs touching this area, including #4670 and #4573 . Ideally, when this goes in, the switch in #4573 can be updated to honour EnableAppConfig and likely that will allow all this to be trimmed out. I'll keep track of it.

[priyankatiwari08]

The approach looks sound: the substitution follows UseManagedNetworking, and EnableAppConfig with a default of true is the right naming choice.

The main gap is coverage: the retry tests initialize the config-reading manager, so they verify provider selection, not that config reads are skipped. Please cover that behavior and the auth-provider/LocalDB gates, and document that the .NET Framework LocalDB gate changes runtime behavior without a trimming benefit.

[priyankatiwari08]

This NotSame forces SqlConfigurableRetryLogicManager to initialize, which runs the config read the switch is meant to avoid — so the test can't distinguish "config not read" from "different provider returned". Compare against SqlConfigurableRetryFactory.CreateNoneRetryProvider's behaviour instead, or assert in a separate process/AppDomain.

[charlesroddie]

Fixed in ea407c1. The disabled tests now compare against the factory's shared no-retry provider instead of the manager.

"Config not read" is now asserted directly: a new test collects SqlClient's trace events while the providers are obtained. Since every path through AppConfigManager.FetchConfigurationSection traces the section name, the absence of such a trace shows app.config wasn't read. The test reads a section deliberately and asserts the trace appears, so it can't pass merely because tracing is unavailable. That made a separate process unnecessary.

[priyankatiwari08]

SqlCommand and SqlConnection each keep their own s_noneRetryProvider, so the disabled path allocates two equivalent none-providers. Hoisting one static onto SqlConfigurableRetryFactory would avoid the duplication.

[charlesroddie]

fixed in cf5b501

[Copilot]

Copilot review overview

🟢 Approval recommended

The switch consistently gates all identified configuration readers, preserves compatibility by default, supports trimming, and has focused retry-path coverage.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🟢 Approval recommended

The switch is consistently gated, documented, substituted for trimming, and covered for default and retry behavior.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

The new switch’s enabled-by-default design conflicts with the repository’s opt-in AppContext switch convention.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Default app.config behavior and switch to opt-in

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​LocalAppContextSwitches.cs:373

This switch violates the repository rule that new AppContext switches default to false and opt in to new behavior (.github/instructions/features.instructions.md:289-294). Since ignoring app.config is the opt-in behavior and existing behavior must remain the default, please name this DisableAppConfig, default it to false, and invert the guards and publish-time setting. ILLink supports substituting the true feature value, so trimming does not require an enabled-by-default switch.

[charlesroddie]

The main gap is coverage: the retry tests initialize the config-reading manager, so they verify provider selection, not that config reads are skipped. Please cover that behavior

Retry is covered in ea407c1. The disabled tests no longer touch the manager, and a new one asserts that no config-read trace is emitted while the providers are obtained.

the auth-provider/LocalDB gates, and document that the .NET Framework LocalDB gate changes runtime behavior without a trimming benefit.

The netframework behaviour is modified in 259bad2, so that netframework is unaffected by the switch. This is documented in the switch table in features.instructions.md. That resolves the localdb part of this which is scoped to netframework: the only read of the system.data.localdb section is LocalDbApi.cs:185, inside a #if NETFRAMEWORK block around CreateLocalDbInstance.

Auth providers are not covered: that reader runs once in a static constructor, so it needs a separate process, and the repo doesn't have a harness for that.