fix: Fix unsafe std::optional accesses (#771)

This fixes unsafe instances of std::optional reported in #755 .

I attempted to pick the solution that made the most sense at each specific instances, so there's a mix of has_value() guards, checked access with .value(), or returning error codes.

Fixes #755

Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp

@@ -785,7 +785,9 @@ class CombinatorialKalmanFilter {
         trackStateProxy.data().ipredicted = neighborState.data().ipredicted;
       } else {
         trackStateProxy.predicted() = boundParams.parameters();
-        trackStateProxy.predictedCovariance() = *boundParams.covariance();
+        if (boundParams.covariance().has_value()) {
+          trackStateProxy.predictedCovariance() = *boundParams.covariance();
+        }
       }
       trackStateProxy.jacobian() = jacobian;
       trackStateProxy.pathLength() = pathLength;
@@ -877,7 +879,9 @@ class CombinatorialKalmanFilter {
       const auto& [boundParams, jacobian, pathLength] = boundState;
       // Fill the track state
       trackStateProxy.predicted() = boundParams.parameters();
-      trackStateProxy.predictedCovariance() = *boundParams.covariance();
+      if (boundParams.covariance().has_value()) {
+        trackStateProxy.predictedCovariance() = *boundParams.covariance();
+      }
       trackStateProxy.jacobian() = jacobian;
       trackStateProxy.pathLength() = pathLength;
       // Set the surface
@@ -923,7 +927,9 @@ class CombinatorialKalmanFilter {
       const auto& [curvilinearParams, jacobian, pathLength] = curvilinearState;
       // Fill the track state
       trackStateProxy.predicted() = curvilinearParams.parameters();
-      trackStateProxy.predictedCovariance() = *curvilinearParams.covariance();
+      if (curvilinearParams.covariance().has_value()) {
+        trackStateProxy.predictedCovariance() = *curvilinearParams.covariance();
+      }
       trackStateProxy.jacobian() = jacobian;
       trackStateProxy.pathLength() = pathLength;
       // Set the surface; reuse the existing curvilinear surface

Core/include/Acts/TrackFinding/MeasurementSelector.hpp

@@ -85,6 +85,11 @@ class MeasurementSelector {
       return CombinatorialKalmanFilterError::MeasurementSelectionFailed;
     }
 
+    // Need covariance to compute chi2
+    if (not predictedParams.covariance().has_value()) {
+      return CombinatorialKalmanFilterError::MeasurementSelectionFailed;
+    }
+
     // Get geoID of this surface
     auto surface = &predictedParams.referenceSurface();
     auto geoID = surface->geometryId();

Core/include/Acts/TrackFitting/KalmanFitter.hpp

@@ -530,8 +530,10 @@ class KalmanFitter {
 
         // Fill the track state
         trackStateProxy.predicted() = std::move(boundParams.parameters());
-        trackStateProxy.predictedCovariance() =
-            std::move(*boundParams.covariance());
+        if (boundParams.covariance().has_value()) {
+          trackStateProxy.predictedCovariance() =
+              std::move(*boundParams.covariance());
+        }
         trackStateProxy.jacobian() = std::move(jacobian);
         trackStateProxy.pathLength() = std::move(pathLength);
 
@@ -637,8 +639,10 @@ class KalmanFitter {
 
             // Fill the track state
             trackStateProxy.predicted() = std::move(boundParams.parameters());
-            trackStateProxy.predictedCovariance() =
-                std::move(*boundParams.covariance());
+            if (boundParams.covariance().has_value()) {
+              trackStateProxy.predictedCovariance() =
+                  std::move(*boundParams.covariance());
+            }
             trackStateProxy.jacobian() = std::move(jacobian);
             trackStateProxy.pathLength() = std::move(pathLength);
           } else if (surface->surfaceMaterial() != nullptr) {
@@ -652,8 +656,10 @@ class KalmanFitter {
             // Fill the track state
             trackStateProxy.predicted() =
                 std::move(curvilinearParams.parameters());
-            trackStateProxy.predictedCovariance() =
-                std::move(*curvilinearParams.covariance());
+            if (curvilinearParams.covariance().has_value()) {
+              trackStateProxy.predictedCovariance() =
+                  std::move(*curvilinearParams.covariance());
+            }
             trackStateProxy.jacobian() = std::move(jacobian);
             trackStateProxy.pathLength() = std::move(pathLength);
           }
@@ -730,8 +736,10 @@ class KalmanFitter {
 
         // Fill the track state
         trackStateProxy.predicted() = std::move(boundParams.parameters());
-        trackStateProxy.predictedCovariance() =
-            std::move(*boundParams.covariance());
+        if (boundParams.covariance().has_value()) {
+          trackStateProxy.predictedCovariance() =
+              std::move(*boundParams.covariance());
+        }
         trackStateProxy.jacobian() = std::move(jacobian);
         trackStateProxy.pathLength() = std::move(pathLength);
 

Core/include/Acts/Utilities/Result.hpp

@@ -295,14 +295,14 @@ class Result<void, E> {
    * @note If `res.ok()` this method will abort (noexcept)
    * @return Reference to the error
    */
-  E& error() & noexcept { return *m_opt; }
+  E& error() & noexcept { return m_opt.value(); }
 
   /**
    * Returns the error by-value.
    * @note If `res.ok()` this method will abort (noexcept)
    * @return Reference to the error
    */
-  E error() && noexcept { return std::move(*m_opt); }
+  E error() && noexcept { return std::move(m_opt.value()); }
 
  private:
   std::optional<E> m_opt;

Core/include/Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp

@@ -111,6 +111,9 @@ auto Acts::AdaptiveGridDensityVertexFinder<trkGridSize, vfitter_t>::
   const double d0 = trk.parameters()[BoundIndices::eBoundLoc0];
   const double z0 = trk.parameters()[BoundIndices::eBoundLoc1];
   // Get track covariance
+  if (not trk.covariance().has_value()) {
+    return false;
+  }
   const auto perigeeCov = *(trk.covariance());
   const double covDD =
       perigeeCov(BoundIndices::eBoundLoc0, BoundIndices::eBoundLoc0);

Core/include/Acts/Vertexing/AdaptiveGridTrackDensity.ipp

@@ -63,7 +63,7 @@ std::pair<int,
 Acts::AdaptiveGridTrackDensity<trkGridSize>::addTrack(
     const Acts::BoundTrackParameters& trk, std::vector<float>& mainGridDensity,
     std::vector<int>& mainGridZValues) const {
-  SymMatrix2 cov = trk.covariance()->block<2, 2>(0, 0);
+  SymMatrix2 cov = trk.covariance().value().block<2, 2>(0, 0);
   float d0 = trk.parameters()[0];
   float z0 = trk.parameters()[1];
 

Core/include/Acts/Vertexing/GaussianGridTrackDensity.ipp

@@ -57,7 +57,7 @@ std::pair<int, typename Acts::GaussianGridTrackDensity<
                    mainGridSize, trkGridSize>::TrackGridVector>
 Acts::GaussianGridTrackDensity<mainGridSize, trkGridSize>::addTrack(
     const Acts::BoundTrackParameters& trk, MainGridVector& mainGrid) const {
-  SymMatrix2 cov = trk.covariance()->block<2, 2>(0, 0);
+  SymMatrix2 cov = trk.covariance().value().block<2, 2>(0, 0);
   float d0 = trk.parameters()[0];
   float z0 = trk.parameters()[1];
 

Core/include/Acts/Vertexing/GaussianTrackDensity.hpp

@@ -139,10 +139,10 @@ class GaussianTrackDensity {
   /// @param trackList All input tracks
   /// @param extractParameters Function extracting BoundTrackParameters from
   /// InputTrack
-  void addTracks(State& state,
-                 const std::vector<const input_track_t*>& trackList,
-                 const std::function<BoundTrackParameters(input_track_t)>&
-                     extractParameters) const;
+  Result<void> addTracks(
+      State& state, const std::vector<const input_track_t*>& trackList,
+      const std::function<BoundTrackParameters(input_track_t)>&
+          extractParameters) const;
 
   /// @brief Evaluate the density function and its two first
   /// derivatives at the specified coordinate along the beamline
@@ -197,5 +197,6 @@ class GaussianTrackDensity {
   };
 };
 
-#include "Acts/Vertexing/GaussianTrackDensity.ipp"
 }  // namespace Acts
+
+#include "Acts/Vertexing/GaussianTrackDensity.ipp"

Core/include/Acts/Vertexing/GaussianTrackDensity.ipp

@@ -6,15 +6,22 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+#include "Acts/Vertexing/VertexingError.hpp"
+
 #include <math.h>
 
+namespace Acts {
+
 template <typename input_track_t>
 std::pair<double, double>
 Acts::GaussianTrackDensity<input_track_t>::globalMaximumWithWidth(
     State& state, const std::vector<const input_track_t*>& trackList,
     const std::function<BoundTrackParameters(input_track_t)>& extractParameters)
     const {
-  addTracks(state, trackList, extractParameters);
+  auto result = addTracks(state, trackList, extractParameters);
+  if (not result.ok()) {
+    return std::make_pair(0., 0.);
+  }
 
   double maxPosition = 0.;
   double maxDensity = 0.;
@@ -68,7 +75,7 @@ double Acts::GaussianTrackDensity<input_track_t>::globalMaximum(
 }
 
 template <typename input_track_t>
-void Acts::GaussianTrackDensity<input_track_t>::addTracks(
+Result<void> Acts::GaussianTrackDensity<input_track_t>::addTracks(
     State& state, const std::vector<const input_track_t*>& trackList,
     const std::function<BoundTrackParameters(input_track_t)>& extractParameters)
     const {
@@ -78,6 +85,9 @@ void Acts::GaussianTrackDensity<input_track_t>::addTracks(
     const double d0 = boundParams.parameters()[BoundIndices::eBoundLoc0];
     const double z0 = boundParams.parameters()[BoundIndices::eBoundLoc1];
     // Get track covariance
+    if (not boundParams.covariance().has_value()) {
+      return VertexingError::NoCovariance;
+    }
     const auto perigeeCov = *(boundParams.covariance());
     const double covDD =
         perigeeCov(BoundIndices::eBoundLoc0, BoundIndices::eBoundLoc0);
@@ -117,6 +127,7 @@ void Acts::GaussianTrackDensity<input_track_t>::addTracks(
     state.trackEntries.emplace_back(z0, constantTerm, linearTerm, quadraticTerm,
                                     zMin, zMax);
   }
+  return Result<void>::success();
 }
 
 template <typename input_track_t>
@@ -162,3 +173,5 @@ void Acts::GaussianTrackDensity<input_track_t>::GaussianTrackDensityStore::
     m_secondDerivative += 2. * entry.c2 * delta + qPrime * deltaPrime;
   }
 }
+
+}  // namespace Acts

Core/include/Acts/Vertexing/GridDensityVertexFinder.ipp

@@ -107,6 +107,9 @@ auto Acts::GridDensityVertexFinder<mainGridSize, trkGridSize, vfitter_t>::
   const double d0 = trk.parameters()[BoundIndices::eBoundLoc0];
   const double z0 = trk.parameters()[BoundIndices::eBoundLoc1];
   // Get track covariance
+  if (not trk.covariance().has_value()) {
+    return false;
+  }
   const auto perigeeCov = *(trk.covariance());
   const double covDD =
       perigeeCov(BoundIndices::eBoundLoc0, BoundIndices::eBoundLoc0);

Core/include/Acts/Vertexing/HelicalTrackLinearizer.ipp

@@ -43,16 +43,16 @@ Acts::Result<Acts::LinearizedTrack> Acts::
     positionAtPCA[ePos2] = pos[ePos2];
     positionAtPCA[eTime] = endParams->time();
   }
-  BoundSymMatrix parCovarianceAtPCA = *(endParams->covariance());
+  BoundSymMatrix parCovarianceAtPCA = endParams->covariance().value();
 
-  if (endParams->covariance()->determinant() <= 0) {
+  if (parCovarianceAtPCA.determinant() <= 0) {
     // Use the original parameters
     paramsAtPCA = params.parameters();
     auto pos = endParams->position(gctx);
     positionAtPCA[ePos0] = pos[ePos0];
     positionAtPCA[ePos1] = pos[ePos1];
     positionAtPCA[ePos2] = pos[ePos2];
-    parCovarianceAtPCA = *(params.covariance());
+    parCovarianceAtPCA = params.covariance().value();
   }
 
   // phiV and functions

Core/include/Acts/Vertexing/ImpactPointEstimator.ipp

@@ -115,6 +115,9 @@ Acts::ImpactPointEstimator<input_track_t, propagator_t, propagator_options_t>::
                       vertexLocPlane.dot(yDirPlane)};
 
   // track covariance
+  if (not trkParams->covariance().has_value()) {
+    return VertexingError::NoCovariance;
+  }
   auto cov = trkParams->covariance();
   SymMatrix2 myWeightXY = cov->block<2, 2>(0, 0).inverse();
 
@@ -286,6 +289,9 @@ Acts::ImpactPointEstimator<input_track_t, propagator_t, propagator_options_t>::
   SymMatrix2 vrtXYCov = vtx.covariance().template block<2, 2>(0, 0);
 
   // Covariance of perigee parameters after propagation to perigee surface
+  if (not propRes.endParameters->covariance().has_value()) {
+    return VertexingError::NoCovariance;
+  }
   const auto& perigeeCov = *(propRes.endParameters->covariance());
 
   Vector2 d0JacXY(-sinPhi, cosPhi);

Examples/Framework/src/Validation/ResPlotTool.cpp

@@ -144,7 +144,6 @@ void ActsExamples::ResPlotTool::fill(
 
   // get the fitted parameter (at perigee surface) and its error
   auto trackParameter = fittedParamters.parameters();
-  auto covariance = *fittedParamters.covariance();
 
   // get the perigee surface
   auto pSurface = &fittedParamters.referenceSurface();
@@ -184,17 +183,24 @@ void ActsExamples::ResPlotTool::fill(
                            residual);
     PlotHelpers::fillHisto(resPlotCache.res_vs_pT.at(parName), truthPt,
                            residual);
-    if (covariance(parID, parID) > 0) {
-      float pull = residual / sqrt(covariance(parID, parID));
-      PlotHelpers::fillHisto(resPlotCache.pull[parName], pull);
-      PlotHelpers::fillHisto(resPlotCache.pull_vs_eta.at(parName), truthEta,
-                             pull);
-      PlotHelpers::fillHisto(resPlotCache.pull_vs_pT.at(parName), truthPt,
-                             pull);
+
+    if (fittedParamters.covariance().has_value()) {
+      auto covariance = *fittedParamters.covariance();
+      if (covariance(parID, parID) > 0) {
+        float pull = residual / sqrt(covariance(parID, parID));
+        PlotHelpers::fillHisto(resPlotCache.pull[parName], pull);
+        PlotHelpers::fillHisto(resPlotCache.pull_vs_eta.at(parName), truthEta,
+                               pull);
+        PlotHelpers::fillHisto(resPlotCache.pull_vs_pT.at(parName), truthPt,
+                               pull);
+      } else {
+        ACTS_WARNING("Fitted track parameter :" << parName
+                                                << " has negative covariance = "
+                                                << covariance(parID, parID));
+      }
     } else {
       ACTS_WARNING("Fitted track parameter :" << parName
-                                              << " has negative covariance = "
-                                              << covariance(parID, parID));
+                                              << " has no covariance");
     }
   }
 }

Examples/Io/Root/src/RootTrajectoryParametersWriter.cpp

@@ -201,21 +201,33 @@ ActsExamples::ProcessCode ActsExamples::RootTrajectoryParametersWriter::writeT(
         m_hasFittedParams = true;
         const auto& boundParam = traj.trackParameters(trackTip);
         const auto& parameter = boundParam.parameters();
-        const auto& covariance = *boundParam.covariance();
         m_eLOC0_fit = parameter[Acts::eBoundLoc0];
         m_eLOC1_fit = parameter[Acts::eBoundLoc1];
         m_ePHI_fit = parameter[Acts::eBoundPhi];
         m_eTHETA_fit = parameter[Acts::eBoundTheta];
         m_eQOP_fit = parameter[Acts::eBoundQOverP];
         m_eT_fit = parameter[Acts::eBoundTime];
-        m_err_eLOC0_fit = sqrt(covariance(Acts::eBoundLoc0, Acts::eBoundLoc0));
-        m_err_eLOC1_fit = sqrt(covariance(Acts::eBoundLoc1, Acts::eBoundLoc1));
-        m_err_ePHI_fit = sqrt(covariance(Acts::eBoundPhi, Acts::eBoundPhi));
-        m_err_eTHETA_fit =
-            sqrt(covariance(Acts::eBoundTheta, Acts::eBoundTheta));
-        m_err_eQOP_fit =
-            sqrt(covariance(Acts::eBoundQOverP, Acts::eBoundQOverP));
-        m_err_eT_fit = sqrt(covariance(Acts::eBoundTime, Acts::eBoundTime));
+
+        if (boundParam.covariance().has_value()) {
+          const auto& covariance = *boundParam.covariance();
+          m_err_eLOC0_fit =
+              sqrt(covariance(Acts::eBoundLoc0, Acts::eBoundLoc0));
+          m_err_eLOC1_fit =
+              sqrt(covariance(Acts::eBoundLoc1, Acts::eBoundLoc1));
+          m_err_ePHI_fit = sqrt(covariance(Acts::eBoundPhi, Acts::eBoundPhi));
+          m_err_eTHETA_fit =
+              sqrt(covariance(Acts::eBoundTheta, Acts::eBoundTheta));
+          m_err_eQOP_fit =
+              sqrt(covariance(Acts::eBoundQOverP, Acts::eBoundQOverP));
+          m_err_eT_fit = sqrt(covariance(Acts::eBoundTime, Acts::eBoundTime));
+        } else {
+          m_err_eLOC0_fit = NaNfloat;
+          m_err_eLOC1_fit = NaNfloat;
+          m_err_ePHI_fit = NaNfloat;
+          m_err_eTHETA_fit = NaNfloat;
+          m_err_eQOP_fit = NaNfloat;
+          m_err_eT_fit = NaNfloat;
+        }
       }
 
       // fill the variables for one track to tree

Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp

@@ -115,14 +115,17 @@ void fillTrackState(const TestTrackState& pc, TrackStatePropMask mask,
 
   if (ACTS_CHECK_BIT(mask, TrackStatePropMask::Predicted)) {
     ts.predicted() = pc.predicted.parameters();
+    BOOST_CHECK(pc.predicted.covariance().has_value());
     ts.predictedCovariance() = *(pc.predicted.covariance());
   }
   if (ACTS_CHECK_BIT(mask, TrackStatePropMask::Filtered)) {
     ts.filtered() = pc.filtered.parameters();
+    BOOST_CHECK(pc.filtered.covariance().has_value());
     ts.filteredCovariance() = *(pc.filtered.covariance());
   }
   if (ACTS_CHECK_BIT(mask, TrackStatePropMask::Smoothed)) {
     ts.smoothed() = pc.smoothed.parameters();
+    BOOST_CHECK(pc.smoothed.covariance().has_value());
     ts.smoothedCovariance() = *(pc.smoothed.covariance());
   }
   if (ACTS_CHECK_BIT(mask, TrackStatePropMask::Jacobian)) {
@@ -524,12 +527,15 @@ BOOST_DATA_TEST_CASE(TrackStateProxyStorage, bd::make({1u, 2u}),
   // check that the track parameters are set
   BOOST_CHECK(ts.hasPredicted());
   BOOST_CHECK_EQUAL(ts.predicted(), pc.predicted.parameters());
+  BOOST_CHECK(pc.predicted.covariance().has_value());
   BOOST_CHECK_EQUAL(ts.predictedCovariance(), *pc.predicted.covariance());
   BOOST_CHECK(ts.hasFiltered());
   BOOST_CHECK_EQUAL(ts.filtered(), pc.filtered.parameters());
+  BOOST_CHECK(pc.filtered.covariance().has_value());
   BOOST_CHECK_EQUAL(ts.filteredCovariance(), *pc.filtered.covariance());
   BOOST_CHECK(ts.hasSmoothed());
   BOOST_CHECK_EQUAL(ts.smoothed(), pc.smoothed.parameters());
+  BOOST_CHECK(pc.smoothed.covariance().has_value());
   BOOST_CHECK_EQUAL(ts.smoothedCovariance(), *pc.smoothed.covariance());
 
   // check that the jacobian is set