Remove some trim/AOT warnings: easy cases - #4684
charlesroddie wants to merge 6 commits into
Conversation
- IpAddressPreferenceUtilities: build the name lookup from explicit values instead of Enum.GetValues(Type) (IL3050). - LocalDB: use generic Marshal.GetDelegateForFunctionPointer<T> (IL3050). - SqlDataSourceEnumeratorNativeHelper: replace DataTable.Select with a row scan using DataTable's string comparison (IL2026). - SqlMetaDataFactory: suppress IL2072 on DataColumn creation; schema collection columns are never INullable types. 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.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Pull request overview
Removes four trim/Native AOT warnings without changing the public API.
Changes:
- Replaces reflection-based enum and delegate APIs.
- Replaces
DataTable.Selectduplicate filtering with row scanning. - Suppresses the schema metadata reflection warning.
File summaries
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlMetaDataFactory.cs | Updated as part of this pull request. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/LocalDB.netcore.cs | Updated as part of this pull request. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs | Updated as part of this pull request. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/IpAddressPreferenceUtilities.cs | Updated as part of this pull request. |
Review details
Suppressed comments (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs:173
ContainsInstancenow reimplementsDataTable.Select's duplicate semantics, including culture-sensitive case/kana/width and trailing-space handling, but this path has no automated regression coverage: the existing manual tests only assert a row-count bound and require SQL Browser. Add focused tests around parsing/deduplication, including apostrophes, empty instance names, and the stated comparison rules, so this replacement cannot silently diverge fromDataTable.Select.
private static bool ContainsInstance(DataTable dataTable, string serverName, string instanceName)
{
CompareInfo compareInfo = dataTable.Locale.CompareInfo;
foreach (DataRow row in dataTable.Rows)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs:189
- This trims the values before comparing, which changes the old
DataTable.Selectsemantics: string equality treats trailing spaces as significant, so rows such asHOSTandHOSTwere distinct before but are now treated as duplicates and one is dropped. Please compare the raw values here (or add an explicit regression test if trimming is intentional).
s1?.TrimEnd(' ', '\u3000'),
s2?.TrimEnd(' ', '\u3000'),
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs:152
- Please add a focused regression test for this replacement, covering duplicate server/instance names that differ only by case, kana/width, trailing spaces, and embedded apostrophes. The existing enumerator tests only assert a broad row-count range against a live SQL Browser, so they would not detect a mismatch between this comparator and the former DataTable.Select behavior.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
SqlMetaDataFactory's IL2072 suppression relies on this. Reword its justification to state the condition directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
- SqlMetaDataFactory: move the suppression from ExecuteCommandAsync to an AddColumn helper so it cannot hide other warnings in the method. - SqlDataSourceEnumeratorNativeHelper: make ParseServerEnumString internal and add unit tests for parsing and de-duplication. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataBaseSchemaTest/ConnectionSchemaTest.cs:180
- The new helper is documented only with an inline comment, but the repository's test guidance requires XML summaries for helper methods and parameter documentation where applicable. Replace this comment with XML documentation describing the assertion and the
schemaTableparameter.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/Sql/SqlDataSourceEnumeratorNativeHelperTest.cs:34
- Because this refactor replaces
DataTable.Select's comparison, these tests should cover the remaining comparison semantics encoded inEqualsInTable:IgnoreKanaType,IgnoreWidth, and U+3000 trailing-space trimming. Without representative kana/full-/half-width and ideographic-space cases, a regression in any of those flags would still pass; add those cases to this theory.
[Theory]
[InlineData("server", "inst")]
[InlineData("SERVER", "INST")]
[InlineData("server ", "inst ")]
public void EquivalentEntry_IsDropped(string serverName, string instanceName)
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
- Choose compare options from DataTable.CaseSensitive, as DataTable does. - Test ideographic trailing spaces and full-width names; drop the test for quotes, which valid server and instance names cannot contain. - Add XML summaries required by the testing instructions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified that would block approval.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
Removes the AllPreferences list and static constructor. The test now checks that every enum value's name converts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5a9df4b to
1b4d741
Compare
There was a problem hiding this comment.
🟢 Approval recommended
Reviewed changes have no unresolved blocking issues.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
Reviewed changes have no unresolved blocking issues.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. |
Description
Removes four trim/Native AOT warnings with local changes:
IpAddressPreferenceUtilities: lists the preference names in the lookup's initializer instead of building it withEnum.GetValues(Type)(IL3050).Enum.GetValues<T>isn't available on net462. A unit test checks that every enum value's name converts.LocalDB: uses the genericMarshal.GetDelegateForFunctionPointer<T>(IL3050).SqlDataSourceEnumeratorNativeHelper: replacesDataTable.Selectwith a row scan (IL2026). The comparison matchesDataTable's string equality: options followDataTable.CaseSensitive(this table is case-insensitive, so case, kana type and width are ignored) and trailing spaces are trimmed.ParseServerEnumStringis nowinternaland has unit tests.SqlMetaDataFactory: suppresses IL2072 on a smallAddColumnhelper aroundDataColumnCollection.Add(string, Type).DataColumnneeds the annotation only to find a staticNullmember onINullablecolumn types, which schema collection queries don't return.ConnectionSchemaTestnow asserts this for every schema collection.Trim/AOT warnings with
-p:IsAotCompatible=true(unique by location and code):No public API or behavioral changes for valid server and instance names.
Issues
Part of #1947.
Testing
-p:IsAotCompatible=true; no warnings in the touched files.ConnectionSchemaTest(16 tests), including the new INullable check, passes on net8.0 against SQL Server 2019 LocalDB.SqlDataSourceEnumeratorNativeHelperTest(parsing; de-duplication across case, trailing ASCII and ideographic spaces, and full-width names; instance matching) passes on net462, net8.0 and net10.0.🤖 Generated with Claude Code