Cleanup | Remove More Dead SMI Code (Part 2) #3414
Merged
Conversation
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
…e that false everywhere it was being used to determine what other branches of code can be deleted
…exceptions they will always throw b/c _smiLink will always be null
…sages of the property It can continue to be propagated if we find out the version is always the same...
…nd propagate that forward, allowing us to remove MetadataUtilsSmi.IsValidForSmiVersion and a block of code in SqlDataRecord
…nating the last uses of SmiVersion2008
… `null`, replacing usages of it with `null`
…since it is always passed to itself
…, since it is always passed to itself and is always null
…pagate null to all usages of it
… propagating null to all usages of it Also removing SqlInternalConnectionSmi.SmiConnection since it's never used.
…o it can be eliminated
There was a problem hiding this comment.
Pull Request Overview
This pull request removes dead SMI code and cleans up related internal APIs by eliminating unused SMI context and versioning logic. Key changes include:
- Removing the InOutOfProcHelper and SmiContext-related branches and entire SmiContextFactory/SmiContext files.
- Updating SMI utility methods (ValueUtilsSmi and related classes) to no longer require a context parameter.
- Changing Debug.Assert/Debug.Fail calls and adding TODO markers to signal areas needing further investigation.
Reviewed Changes
Copilot reviewed 17 out of 17 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/SqlUtil.cs | Added a TODO comment to verify method usage. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependency.cs | Removed obsolete InProc SMI checks. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi*.cs | Updated method signatures and XML documentation to remove context parameters. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlDataRecord*.cs | Removed branches using SMI context and simplified legacy logic. |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionSmi.cs | Removed SMI context usage and updated getters to throw as expected. |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs | Replaced Debug.Assert with Debug.Fail for negotiated SMI version checks. |
| Other files | Removed obsolete SMI API files and updated references accordingly. |
Comments suppressed due to low confidence (4)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs:2437
- [nitpick] Consider expanding this TODO comment with specific criteria or conditions under which these methods should be reviewed or removed to improve clarity for future maintenance.
// @TODO: Check these methods for usage
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs:467
- [nitpick] Consider replacing Debug.Fail with a more descriptive logging mechanism or a custom exception so that future debugging efforts clearly understand the expected behavior when this condition is encountered.
Debug.Fail("NegotiatedSmiVersion will always throw (SmiContextFactory.Instance.NegotiatedSmiVersion >= SmiContextFactory.Sql2005Version)");
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionSmi.cs:32
- [nitpick] Add a comment next to this getter to explain why it is expected to always throw, which would help maintainers understand the rationale behind omitting SMI context handling.
get => throw SQL.ContextUnavailableOutOfProc();
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiContextFactory.netfx.cs:1
- Ensure that all references to SmiContextFactory have been updated and removed to avoid linkage errors or unexpected behavior in the absence of this obsolete API.
// Entire file removed
|
There are a few types which are only ever used from these code paths too.
Besides this, SqlSequentialStreamSmi and SqlSequentialTextReaderSmi will also disappear shortly, once SqlDataReaderSmi is removed. |
|
@edwardneal Very good catch!! Let's get that deleted code counter over 1000! |
paulmedynski
approved these changes
Jun 12, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3414 +/- ##
==========================================
- Coverage 68.33% 65.80% -2.53%
==========================================
Files 301 294 -7
Lines 65631 65210 -421
==========================================
- Hits 44849 42913 -1936
- Misses 20782 22297 +1515
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mdaigle
approved these changes
Jun 16, 2025
priyankatiwari08
approved these changes
Jun 17, 2025
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.
Description
While working on another chunk of code, I had another breakthrough on provably deleting SMI code. There's a tiny class
InOutOfProcHelperthat contains a single propertyInProcthat always returnsfalse. Pulling on this thread revealed that the entirety of SmiContextFactory and SmiContext could be deleted.The process I used for safely deleting these was to basically take the hard coded values and propagate them down to lower levels. At each step of propagating, I delete branches of the code became unreachable, and keep pushing it further down. Each commit builds on its own, so it's possible to step through the changes. Trying to explain each step would probably be impossible...
Removing SmiContext from ValueUtilsSmi became a bit tricky since there are circular references that pass an SmiContext between each other. However, knowing that SmiContext should always be null, I was able to work it out by passing null into the calls to the methods, then making the argument have a null default, then eliminating nulls from the calls to the methods, then removing the "internal" usages of the argument since logically they could only be null.
There are a couple annotated spots where the code must've never been called because it will literally always throw.
Testing
These branches should not have been reached previously, so CI should be sufficient validate nothing broke.