Cleanup | Remove SmiEventSink_Default

[edwardneal]

Description

This follows up #3431 - sorry if I'm treading on your toes here @benrr101. It's a fairly bulky set of changes, but most of this bulk comes from removing the methods in ValueUtilsSmi which accepted and passed SmiEventSink_Default parameters. The majority of the work is handled in the first three methods.

My process here is clearest when considered in the context of SmiEventSink_Default.cs:

1. ProcessMessagesAndThrow only does work if called when HasMessages is true.
2. HasMessages is only true if _errors or _warnings is non-null.
3. Both variables are private in scope. They are only ever assigned to in ProcessMessages, and in both cases they're set to null.
4. Based on this, HasMessages is always false, and ProcessMessagesAndThrow never does anything. It can be removed.
5. ProcessMessagesAndThrow was the only member (besides the constructor) accessed on the class.

I thus removed the entire class and every reference to it. This has knock-on effects on SqlDataReader's target-specific files, (which can be merged and removed) but those can be cleaned up later.

Testing

Unit tests and manual tests pass, although I'd appreciate a CI run.

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[benrr101]

Looks great! One small tweak you can make if you feel like it. No worries about stepping on my toes - I kinda thought I had hit the end of the line for removing SMI code, so I'm extra glad there's more to chuck 😄

[Copilot]

Pull Request Overview

This PR removes the obsolete SmiEventSink_Default class and all its usages, simplifying ValueUtilsSmi calls by dropping the event-sink parameter and cleaning up related code.

• Deleted the SmiEventSink_Default class and its compile includes.
• Updated all ValueUtilsSmi calls in SqlDataRecord, stream classes, and TdsParser to remove the sink parameter.
• Removed the _eventSink field and message-processing calls from SqlDataRecord constructor.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiEventSink_Default.cs	Deleted obsolete event-sink class
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.netfx.cs src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.netcore.cs src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs	Removed _eventSink usage and updated ValueUtilsSmi calls
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiSettersStream.cs	Updated constructor and ValueUtilsSmi calls to drop sink parameter
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiGettersStream.cs	Updated constructor and ValueUtilsSmi calls to drop sink parameter
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient/TdsParser.cs	Removed temporary new SmiEventSink_Default() from SetCompatibleValueV200
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj	Removed compile includes for SmiEventSink_Default

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiSettersStream.cs:77

• Consider adding unit tests for SmiSettersStream methods (Flush, SetLength, Write, Seek) to verify correct behavior after removing the event sink parameter.

            _lengthWritten = ValueUtilsSmi.SetBytesLength(_setters, _ordinal, _metaData, _lengthWritten);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs:380

• [nitpick] The _recordBuffer initialization is duplicated in both #if NETFRAMEWORK and #else branches. You can move it above the directives and only keep the _usesStringStorageForXml assignment inside the #if to reduce duplication.

            _recordBuffer = new MemoryRecordBuffer(_columnSmiMetaData);

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[codecov]

Codecov Report

Attention: Patch coverage is 64.19753% with 29 lines in your changes missing coverage. Please review.

Project coverage is 60.14%. Comparing base (b9b9153) to head (910abed).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Microsoft/Data/SqlClient/Server/SqlDataRecord.cs	70.00%	18 Missing ⚠️
...oft/Data/SqlClient/Server/SqlDataRecord.netcore.cs	44.44%	5 Missing ⚠️
...osoft/Data/SqlClient/Server/SqlDataRecord.netfx.cs	40.00%	3 Missing ⚠️
...icrosoft/Data/SqlClient/Server/SmiSettersStream.cs	50.00%	2 Missing ⚠️
...icrosoft/Data/SqlClient/Server/SmiGettersStream.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3438      +/-   ##
==========================================
- Coverage   64.70%   60.14%   -4.57%     
==========================================
  Files         280      274       -6     
  Lines       62174    62065     -109     
==========================================
- Hits        40231    37328    -2903     
- Misses      21943    24737    +2794     

Flag	Coverage Δ
addons	?
netcore	63.34% <65.78%> (-3.74%)	⬇️
netfx	62.18% <66.66%> (-6.03%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.