Skip to content

[WIP] Fix suspicious fragments found by PVS-Studio analyzer - #7696

Draft
rosebyte with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-suspicious-fragments
Draft

[WIP] Fix suspicious fragments found by PVS-Studio analyzer#7696
rosebyte with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-suspicious-fragments

Conversation

Copilot AI commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.


This section details on the original issue you should resolve

<issue_title>Suspicious fragments found by PVS-Studio Static Analyzer</issue_title>
<issue_description>Hey!

I checked the ML.NET framework with the PVS-Studio static code analyzer. I described the suspicious code fragments detected by the analyzer in my article ''ML.NET: can Microsoft's machine learning be trusted?''. I thought you might be interested to read it.</issue_description>

<agent_instructions>analyse the linked blog post and make a PR fixing anything that needs to be fixed</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@rosebyte

rosebyte commented Sep 8, 2026

Copy link
Copy Markdown
Member

@copilot , try again

Copilot AI and others added 2 commits September 8, 2026 11:51
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>

Copilot AI commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot , try again

Implemented fixes in 49fe5d6 and 1f8c021: EstimatorChain and TransformerChain now use their null-normalized arrays, also avoiding double enumeration of transformer input. All eight targeted tests pass; the regression cases fail without the fixes. Read-only review found no significant issues.

The full article and mirrors remain inaccessible from this environment, so this is a partial resolution. Please attach the article’s remaining findings so I can verify them. CodeQL was attempted but skipped because the database is too large.

Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.89%. Comparing base (2fc13ba) to head (12fa2bd).

Files with missing lines Patch % Lines
test/Microsoft.ML.Tests/ChainTests.cs 95.91% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7696      +/-   ##
==========================================
+ Coverage   69.88%   69.89%   +0.01%     
==========================================
  Files        1487     1488       +1     
  Lines      276253   276347      +94     
  Branches    28291    28296       +5     
==========================================
+ Hits       193053   193158     +105     
+ Misses      75710    75707       -3     
+ Partials     7490     7482       -8     
Flag Coverage Δ
Debug 69.89% <98.01%> (+0.01%) ⬆️
production 64.07% <100.00%> (+<0.01%) ⬆️
test 89.83% <97.91%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
...c/Microsoft.ML.Data/DataLoadSave/EstimatorChain.cs 95.00% <100.00%> (+5.00%) ⬆️
...Microsoft.ML.Data/DataLoadSave/TransformerChain.cs 89.47% <100.00%> (+1.16%) ⬆️
src/Microsoft.ML.Ensemble/PipelineEnsemble.cs 79.52% <100.00%> (+0.65%) ⬆️
src/Microsoft.ML.FastTree/BoostingFastTree.cs 60.00% <ø> (+3.13%) ⬆️
src/Microsoft.ML.Transforms/Text/LdaSingleBox.cs 67.67% <100.00%> (ø)
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 88.44% <100.00%> (ø)
...crosoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs 98.45% <100.00%> (+<0.01%) ⬆️
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs 97.90% <100.00%> (+0.05%) ⬆️
test/Microsoft.ML.Tests/ChainTests.cs 95.91% <95.91%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rosebyte

rosebyte commented Sep 9, 2026

Copy link
Copy Markdown
Member

@copilot, tighten #7696 based on the review below. The aim is a small, worthwhile PR, not to fix every warning in the original article.

Keep these changes:

  • TransformerChain: use the materialised array for LastTransformer. This fixes genuine public-API behaviour: single-use or consuming enumerables can fail on a second enumeration, and recreating enumerables can make LastTransformer reference an object not stored in the chain. Keep the public null/empty and single-enumeration tests.
  • PipelineEnsemble: keep the null-safe metadata comparison and its focused coverage. This improves diagnostics for invalid metadata; it does not make previously invalid models usable.
  • LDA: keep both argument-order changes together. The old swaps cancelled each other, so this is behaviour-preserving cleanup, not a fix to training, model loading or native argument values.

Prune low-value coverage:

  • Remove EstimatorChainAcceptsNullOrEmptyEstimators from ChainTests.cs. It uses reflection to manufacture a null input to a private constructor that supported construction paths never supply. Do not replace it with another private-implementation test. Keeping the one-line EstimatorChain production cleanup is fine.
  • Reduce FastTreeHistogramPoolSizeValidation to the oversized case (5, 5) and valid upper boundary (5, 4). The other cases cover unchanged normalisation behaviour and are unnecessary for this diagnostic-only change. Keep the duplicate-condition removal.

Correct the PR’s claims:

  • Replace the placeholder description and use an accurate title.
  • Distinguish the TransformerChain correctness fix, diagnostic improvements and behaviour-preserving cleanup. Do not claim broad performance gains or repaired LDA behaviour.
  • Remove all “Fixes” / “Closes” references to Suspicious fragments found by PVS-Studio Static Analyzer #6321. Use “Related to Suspicious fragments found by PVS-Studio Static Analyzer #6321” and explicitly describe this as a partial resolution.
  • Briefly acknowledge that original findings remain: LdaTransform.Train dereferences a failed vector cast when constructing its scalar-input schema error; TreeEnsembleCombiner still dereferences a null calibrated model for uncalibrated inputs; ProgressReporter still transposes subchannel ID and depth. These are pre-existing issues, not regressions introduced by this PR.
  • Do not expand this PR to address those remaining findings. Do not blindly implement the article’s proposed fixes; some depend on caller invariants or apparently dormant code.

Keep the edits surgical and update the existing PR rather than opening another one. Run the smallest relevant existing tests for the changed coverage, using available build prerequisites; do not launch a full-repository build. If validation is blocked, report the blocker rather than claiming success.

Summarise what you retained, removed and corrected in the PR description. Use British English and no em dashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suspicious fragments found by PVS-Studio Static Analyzer

2 participants