clang-tidy: resolve bugprone-unchecked-optional-access#495
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses clang-tidy’s bugprone-unchecked-optional-access findings by suppressing false positives in Catch2 tests and by adding a defensive fallback path in the Python module wrapper when an expected Python error message cannot be retrieved.
Changes:
- Add
NOLINTNEXTLINE(bugprone-unchecked-optional-access)annotations in Catch2 tests whereREQUIRE(...)guardsstd::optionaldereference. - Adjust a test to use
REQUIRE(item)beforeitem->...and suppress the correspondingclang-tidywarning. - Add a fallback
std::logic_errorthrow inplugins/python/src/modulewrap.cppfor the “no message available” error-reporting edge case, with a coverage exclusion marker.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/identifier.cpp |
Suppresses bugprone-unchecked-optional-access for an optional dereference guarded by REQUIRE. |
test/form/form_basics_test.cpp |
Uses REQUIRE(item) and suppresses clang-tidy for guarded optional member access. |
plugins/python/src/modulewrap.cpp |
Adds a defensive fallback exception when Python error text cannot be extracted. |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #495 +/- ##
==========================================
+ Coverage 85.54% 85.59% +0.05%
==========================================
Files 142 142
Lines 3604 3604
Branches 616 615 -1
==========================================
+ Hits 3083 3085 +2
Misses 310 310
+ Partials 211 209 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
6a806b1 to
c5623fb
Compare
- `test/form/form_basics_test.cpp` - `test/identifier.cpp` `clang-tidy` does not recognize Catch2's `REQUIRE` macro as protecting the target `std::optional` against non-existent-value access; use `// NOLINT` to silence the warning. - `plugins/python/src/modulewrap.cpp` `clang-tidy` correctly noted that there was a (theoretical) path to unprotected access to an `std::optional`. We now throw an `std::logic_error` and protect it from coverage validation as a `should never happen` untestable case.
Claude Sonnet 4.6 claims that `msg_from_py_error()` _will_ return true when called if `!opq.has_value()`, so an else clause would be structurally unreachable code. Additionally, the claim is that the `std::runtime_error()` throw will never be caught, and we should probably be using `Python_SetErr()` here and returning `nullptr` instead of throwing exceptions.
8ccdc78 to
e2f8495
Compare
test/form/form_basics_test.cpptest/identifier.cppclang-tidydoes not recognize Catch2'sREQUIREmacro as protectingthe target
std::optionalagainst non-existent-value access; use// NOLINTto silence the warning.plugins/python/src/modulewrap.cppclang-tidycorrectly noted that there was a (theoretical) path tounprotected access to an
std::optional. We now throw anstd::logic_errorand protect it from coverage validation as ashould never happenuntestable case.