Fix and improve array and mdspan static analysis warnings by StephanTLavavej · Pull Request #4856 · microsoft/STL (original) (raw)

🗺️ Overview

Fixes DevCom-10342063 VSO-1804139 "False positive C28020 iterating over single element std::array". Thanks to Hwi-sung Im from the static analysis team for explaining how to fix this with _Ret_range_(==, _Size).

The issue is that we had SAL annotations on array::operator[] explaining its precondition, but static analysis didn't know that array<T, N>::size() always returns N. (By design, it doesn't do interprocedural analysis.) Adding a return annotation allows /analyze to understand loops that are guarded by arr.size().

Updating array then revealed that <mdspan> (which uses array extensively) should be updated accordingly.

📜 Commits

• Update product code.
  • Mark array::size() with _Ret_range_(==, _Size), which is what fixes the reported bug.
  • As a cleanup, replace _In_range_(0, _Size - 1) with _In_range_(<, _Size). I wasn't previously aware of this form, but it appears elsewhere in the MSVC codebase, and is much more natural as it aligns with our _STL_VERIFY checks.
    * Note: For array and <mdspan> below, we're always working with size_t, so we don't need to worry about negative numbers.
  • In <mdspan>, update a comment from citing DevCom-923103 to saying "guaranteed by how _Rank_dynamic is calculated". We have an ironclad guarantee here, but the proof is way more subtle than static analysis can reasonably be expected to understand, so this isn't a compiler bug workaround.
  • Update rank() with _Ret_range_ and static_extent(), extent(), and stride() with _In_range_.
  • Add a missing check to layout_stride::mapping::stride().
  • Note that mdspan::extent() isn't missing a check, it's just performed by this->_Map.extents().extent(_Idx) and the message will be sufficiently relevant.
• Update libcxx skips.
  • Update the comment to explain that static analysis isn't exactly wrong here, it just can't see the guarantees. It's not immediately clear to me whether we can improve things further without suppressing the warning, hence it's still "Not analyzed".
  • Update the quoted warning message, affected by the new usage of _In_range_(<, _Rank).
  • Several tests no longer need to be marked as FAIL, but one needs to be added (because we have more precondition annotations).
• Add new test coverage.
  • Simplified from the original report. I added both the 1-element case (which warned) and a many-element case (which didn't), to be comprehensive. I verified that this emitted the analysis warning before the product code changes. This is a compile-only test.
• Update std test coverage.
  • In test_mdspan_support.hpp, add assert(i < rank) so static analysis is happy with the precondition. (Attempting to propagate the annotation to the lambda led to more warnings, since it's used with invoke() and iota_view.)
  • In the mdspan layout tests, we no longer need to suppress the warning, since Ext::rank() is now understood.
  • Change P0009R18_mdspan_layout_stride to use Ext::rank() instead of the equivalent strs.size() so /analyze can see the guarantee.
  • In P0218R1_filesystem, the EXPECT macro calls a function that returns the given bool, but /analyze doesn't see that, so it doesn't realize we're checking the validity of index. Restructure this with a direct test and EXPECT(false) (which will still result in decent logging since the line number is captured).
• Update death test coverage.
  • As in a couple of pre-existing cases, we now need to suppress the /analyze warning when intentionally making out-of-bounds calls. I mention /analyze instead of the internal "PREfast" terminology here.
• Cleanup std test coverage.
  • These suppressions are still necessary because the iterator-based checking doesn't let the static analyzer understand the bounds guarantees. I'm updating the quoted warning messages as previously mentioned. I'm also replacing suppress with push/disable/pop, as we've already completely done in the product code, since suppress is annoyingly fragile and we should avoid it, despite the additional lines needed to push/disable/pop.