[edwardneal]

Description

This PR performs final (non-source-breaking) resync between the ref and the implementation SqlClient assemblies, using ApiCompat between the two assemblies to prevent future drift in most situations.

The curated NuGet feed needs to have the Microsoft.DotNet.ApiCompat.Task package in it before CI will pass.

Build changes

Since we need to have a ref assembly to compare against, I've added a project reference to it from the implementation assembly. This follows the same pattern as the notsupported assembly.

ApiCompat always runs, and runs in its strictest configuration. This forces all types and attributes (besides a few suppressions) to match between the two assemblies, and prevents parameter name changes.

ApiCompat exclusions/suppressions

Most of the attribute exclusions are fairly non-controversial. The attributes generated by Roslyn for async/iterator state machines and for compiler-generated code are ignored, as are the attributes generated to handle nullable reference types.

I've currently chosen to exclude two other groups: DynamicallyAccessedMembers and TypeForwardedFrom. DynamicallyAccessedMembers is excluded because NativeAOT support isn't fully implemented; this attribute is present on types and parameters which perform UDT handling, and this isn't fully settled on yet. Future NativeAOT work will need to adjust this exclusion.

TypeForwardedFrom clashes slightly with #4816. I'd originally excluded the attribute because I saw the ref assemblies as compile-time concerns and the type forwarding as runtime concerns. I'd be just as happy to drop this exclusion and incorporate the type forwarding into the ref assembly.

The mismatched type forwarding also explains the wider set of type suppressions in ApiCompatSuppressions[.netcore].xml.

Changes

Generally speaking, I've aligned the ref assembly with the runtime assembly unless doing so would mean deleting an attribute. There are only a few such situations, but where this has happened I've added it to the implementation assembly. The single exception to both rules is SqlCommand.ResetCommandTimeout - this attribute has no effect on a Reset method, and the implementation never had it.

Reference assembly changes:

• netfx: add ComVisible to SqlFileStream.CanTimeout, ReadTimeout and WriteTimeout.
• netcore: add StringSyntax to the SqlJson constructor.
• Add DesignerCategory to SqlCommand, SqlCommandBuilder, SqlConnection, SqlDataAdapter.
• Change DefaultValue to use the enum name rather than an integer: SqlCommand.CommandType, SqlCommand.UpdatedRowSource.
• Remove DesignerSerializationVisibility from SqlCommand.ResetCommandTimeout - this was not valid on this method. While the ResetX notation can be valid, it doesn't require this attribute.
• Add DesignerSerializationVisibility to SqlCommand.Parameters, SqlCommand.RetryLogicProvider, SqlConnection.ServerProcessId, SqlConnection.RetryLogicProvider.
• Add Browsable to SqlCommand.RetryLogicProvider, SqlConnection.ServerProcessId, SqlConnection.RetryLogicProvider. This hides them from a winforms PropertyGrid.
• Promote DefaultValue from conditionally compiled (netfx-only): SqlConnection.ColumnEncryptionKeyCacheTtl, SqlConnection.ColumnEncryptionQueryMetadataCacheEnabled, SqlConnection.ColumnEncryptionTrustedMasterKeyPaths.
• Add DefaultValue: SqlParameter.ForceColumnEncryption.
• Add TypeConverter and associated non-public converter types: SqlConnectionEncryptOption, SqlConnectionStringBuilder, SqlConnectionStringBuilder.InitialCatalog, SqlConnectionStringBuilder.NetworkLibrary, SqlParameter.
• Add Flags: SqlConnectionOverrides.
• Change message on Obsolete: SqlConnectionStringBuilder.ConnectionReset (removed a double-space.)
• Change DefaultProperty to correct name: SqlConnectionStringBuilder.

Implementation assembly changes:

• Promote DesignerSerializationVisibility from conditionally compiled (netfx-only): SqlException.Errors.
• Add DesignerSerializationVisibility to SqlParameterCollection.this[].
• Add Browsable to SqlParameter.Offset and SqlParameterCollection.this[].
• Add DefaultValue to SqlParameter.IsNullable.
• Add EditorBrowsable to SqlParameter.Offset.

Known limitation

This works well for net462 and net10.0, but netstandard2.0 has no implementation assemblies to compare against, and the per-TFM API surface varies. This means that we can't use ApiCompat for that TFM, and can't practically compare between the netstandard2.0 and one of the other builds.

Issues

Contributes to #4826. That'll be ready to close once this is merged and a decision is made on backports.

Testing

Compile-time changes should be validated by CI - the implementation-side changes are limited to attribute metadata.

[azure-pipelines]

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

[priyankatiwari08]

Looks good — no code concerns. CodeQL build currently fails with NU1301 until Microsoft.DotNet.ApiCompat.Task is in the curated feed (as noted in the description).

[cheenamalhotra]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[cheenamalhotra]

Scope: 91395855..3376d0d0 (full PR diff)

What this PR does: Turns on ApiCompat so the implementation assembly is checked against the ref assembly for net462 and net10.0. It also lines up attributes and adds suppressions for type forwards.

Existing feedback

Source	Item	Status
@priyankatiwari08 (review)	CodeQL fails with NU1301 until Microsoft.DotNet.ApiCompat.Task is in the curated feed	Feed blocker, not a code issue
Copilot	none	n/a

New findings

Priority	Location	Issue
P2	Microsoft.Data.SqlClient.csproj:73	ApiCompat is skipped when building only one framework (-f net10.0)

Tests: CI didn't validate this because of the feed blocker. The finding comes from tracing the ApiCompat task targets. I didn't run a local build. The milestone check failure is stale now that 8.0.0-preview1 is set.

[cheenamalhotra]

@edwardneal there are some build errors that would need your attention. Thanks.

[edwardneal]

Thanks @cheenamalhotra, looks like it's the result of the earlier PR being merged. I've merged main, finished the removal of the type forwarding attributes and confirmed that the build passes locally.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.62%. Comparing base (cb66150) to head (0ec53c2).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4890      +/-   ##
==========================================
- Coverage   67.91%   66.62%   -1.29%     
==========================================
  Files         311      305       -6     
  Lines       45286    68341   +23055     
==========================================
+ Hits        30757    45534   +14777     
- Misses      14529    22807    +8278     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	66.62% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[cheenamalhotra]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.