Additional index group checks for selections

Now index groups provided to selections are checked more thoroughly, and
an error is given if a group references an atom index outside the
topology (or the maximum atom number in the absence of a topology).
Negative atom indices also trigger the same error.

Previously, such index groups were silently accepted (except for a
warning during reading for negative indices), and the out-of-range atoms
dropped from the selections.

Change-Id: Id7a024ed316920d9594454ae36f7b9bdd9a429fc

diff --git a/src/gromacs/selection/indexutil.cpp b/src/gromacs/selection/indexutil.cpp
index f921caeaac856565f63e953a9df31505d35d9589..3ed8c8783e99020e382325d528546265396e4c61 100644 (file)
--- a/src/gromacs/selection/indexutil.cpp
+++ b/src/gromacs/selection/indexutil.cpp
@@ -440,6 +440,19 @@ gmx_ana_index_check_sorted(gmx_ana_index_t *g)
     return true;
 }
 
+bool
+gmx_ana_index_check_range(gmx_ana_index_t *g, int natoms)
+{
+    for (int i = 0; i < g->isize; ++i)
+    {
+        if (g->index[i] < 0 || g->index[i] >= natoms)
+        {
+            return false;
+        }
+    }
+    return true;
+}
+
 /********************************************************************
  * Set operations
  ********************************************************************/

diff --git a/src/gromacs/selection/indexutil.h b/src/gromacs/selection/indexutil.h
index f9b4b3f46cae5af218cb55e3b825da0ec55eb0dc..4575879009167e905247d771840d5fc73b1f331a 100644 (file)
--- a/src/gromacs/selection/indexutil.h
+++ b/src/gromacs/selection/indexutil.h
@@ -239,6 +239,17 @@ gmx_ana_index_dump(FILE *fp, gmx_ana_index_t *g, int maxn);
 /** Checks whether an index group is sorted. */
 bool
 gmx_ana_index_check_sorted(gmx_ana_index_t *g);
+
+/*! \brief
+ * Checks whether an index group has atoms from a defined range.
+ *
+ * \param[in]  g      Index group to check.
+ * \param[in]  natoms Largest atom number allowed.
+ * \returns    true if all atoms in the index group are in the
+ *     range 0 to \p natoms (i.e., no atoms over \p natoms are referenced).
+ */
+bool
+gmx_ana_index_check_range(gmx_ana_index_t *g, int natoms);
 /*@}*/
 
 /*! \name Functions for set operations on gmx_ana_index_t

diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp
index 14f55a9a6aac7f521e5e61b5a8b4e49dbd04b8c4..3ba3e572a9b0708afeca7eb67fdc7feb1b52acb0 100644 (file)
--- a/src/gromacs/selection/parsetree.cpp
+++ b/src/gromacs/selection/parsetree.cpp
@@ -915,8 +915,9 @@ _gmx_sel_init_group_by_name(const char *name, yyscan_t scanner)
 
     if (_gmx_sel_lexer_has_groups_set(scanner))
     {
-        gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner);
-        sel->resolveIndexGroupReference(grps);
+        gmx_ana_indexgrps_t     *grps = _gmx_sel_lexer_indexgrps(scanner);
+        gmx_ana_selcollection_t *sc   = _gmx_sel_lexer_selcollection(scanner);
+        sel->resolveIndexGroupReference(grps, sc->gall.isize);
     }
 
     return sel;
@@ -938,8 +939,9 @@ _gmx_sel_init_group_by_id(int id, yyscan_t scanner)
 
     if (_gmx_sel_lexer_has_groups_set(scanner))
     {
-        gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner);
-        sel->resolveIndexGroupReference(grps);
+        gmx_ana_indexgrps_t     *grps = _gmx_sel_lexer_indexgrps(scanner);
+        gmx_ana_selcollection_t *sc   = _gmx_sel_lexer_selcollection(scanner);
+        sel->resolveIndexGroupReference(grps, sc->gall.isize);
     }
 
     return sel;

diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp
index 8500bc1518af27117a6f3525b0d2640200d4df4d..2d766bcdf4936feeaa3eb2106c714bcf8c26b325 100644 (file)
--- a/src/gromacs/selection/selectioncollection.cpp
+++ b/src/gromacs/selection/selectioncollection.cpp
@@ -423,6 +423,44 @@ early_termination:
     return result;
 }
 
+/*! \brief
+ * Checks that index groups have valid atom indices.
+ *
+ * \param[in]    root    Root of selection tree to process.
+ * \param[in]    natoms  Maximum number of atoms that the selections are set
+ *     to evaluate.
+ * \param        errors  Object for reporting any error messages.
+ * \throws std::bad_alloc if out of memory.
+ *
+ * Recursively checks the selection tree for index groups.
+ * Each found group is checked that it only contains atom indices that match
+ * the topology/maximum number of atoms set for the selection collection.
+ * Any issues are reported to \p errors.
+ */
+void checkExternalGroups(const SelectionTreeElementPointer &root,
+                         int                                natoms,
+                         ExceptionInitializer              *errors)
+{
+    if (root->type == SEL_CONST && root->v.type == GROUP_VALUE)
+    {
+        try
+        {
+            root->checkIndexGroup(natoms);
+        }
+        catch (const UserInputError &)
+        {
+            errors->addCurrentExceptionAsNested();
+        }
+    }
+
+    SelectionTreeElementPointer child = root->child;
+    while (child)
+    {
+        checkExternalGroups(child, natoms, errors);
+        child = child->next;
+    }
+}
+
 }   // namespace
 
 
@@ -435,7 +473,7 @@ void SelectionCollection::Impl::resolveExternalGroups(
     {
         try
         {
-            root->resolveIndexGroupReference(grps_);
+            root->resolveIndexGroupReference(grps_, sc_.gall.isize);
         }
         catch (const UserInputError &)
         {
@@ -552,6 +590,20 @@ SelectionCollection::setTopology(t_topology *top, int natoms)
     {
         natoms = top->atoms.nr;
     }
+    if (impl_->bExternalGroupsSet_)
+    {
+        ExceptionInitializer        errors("Invalid index group references encountered");
+        SelectionTreeElementPointer root = impl_->sc_.root;
+        while (root)
+        {
+            checkExternalGroups(root, natoms, &errors);
+            root = root->next;
+        }
+        if (errors.hasNestedExceptions())
+        {
+            GMX_THROW(InconsistentInputError(errors));
+        }
+    }
     gmx_ana_selcollection_t *sc = &impl_->sc_;
     // Do this first, as it allocates memory, while the others don't throw.
     gmx_ana_index_init_simple(&sc->gall, natoms);

diff --git a/src/gromacs/selection/selelem.cpp b/src/gromacs/selection/selelem.cpp
index a10d5a5802af892480c4664f5984b2e459165660..b396fa1a1127403309e48a3f1e2f4f9c149db8c1 100644 (file)
--- a/src/gromacs/selection/selelem.cpp
+++ b/src/gromacs/selection/selelem.cpp
@@ -366,7 +366,8 @@ void SelectionTreeElement::checkUnsortedAtoms(
     }
 }
 
-void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps)
+void SelectionTreeElement::resolveIndexGroupReference(
+        gmx_ana_indexgrps_t *grps, int natoms)
 {
     GMX_RELEASE_ASSERT(type == SEL_GROUPREF,
                        "Should only be called for index group reference elements");
@@ -411,6 +412,26 @@ void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps)
     gmx_ana_index_set(&u.cgrp, foundGroup.isize, foundGroup.index,
                       foundGroup.nalloc_index);
     setName(foundName);
+
+    if (natoms > 0)
+    {
+        checkIndexGroup(natoms);
+    }
+}
+
+void SelectionTreeElement::checkIndexGroup(int natoms)
+{
+    GMX_RELEASE_ASSERT(type == SEL_CONST && v.type == GROUP_VALUE,
+                       "Should only be called for index group elements");
+    if (!gmx_ana_index_check_range(&u.cgrp, natoms))
+    {
+        std::string message = formatString(
+                    "Group '%s' cannot be used in selections, because it "
+                    "contains negative atom indices and/or references atoms "
+                    "not present (largest allowed atom index is %d).",
+                    name().c_str(), natoms);
+        GMX_THROW(InconsistentInputError(message));
+    }
 }
 
 } // namespace gmx

diff --git a/src/gromacs/selection/selelem.h b/src/gromacs/selection/selelem.h
index 13c0e949f8314aaada316df89d35fc65dd11ea6d..1a1e86bf611d7644c65de9dcf8748dfb27d0f97e 100644 (file)
--- a/src/gromacs/selection/selelem.h
+++ b/src/gromacs/selection/selelem.h
@@ -339,14 +339,24 @@ class SelectionTreeElement
         void checkUnsortedAtoms(bool                  bUnsortedAllowed,
                                 ExceptionInitializer *errors) const;
         /*! \brief
-         * Resolved an unresolved reference to an index group.
+         * Resolves an unresolved reference to an index group.
          *
-         * \param[in] grps  Index groups to use to resolve the reference.
+         * \param[in] grps   Index groups to use to resolve the reference.
+         * \param[in] natoms Maximum number of atoms the selections can evaluate to
+         *     (zero if the topology/atom count is not set yet).
          * \throws    std::bad_alloc if out of memory.
          * \throws    InconsistentInputError if the reference cannot be
          *     resolved.
          */
-        void resolveIndexGroupReference(gmx_ana_indexgrps_t *grps);
+        void resolveIndexGroupReference(gmx_ana_indexgrps_t *grps, int natoms);
+        /*! \brief
+         * Checks that an index group has valid atom indices.
+         *
+         * \param[in] natoms Maximum number of atoms the selections can evaluate to.
+         * \throws    std::bad_alloc if out of memory.
+         * \throws    InconsistentInputError if there are invalid atom indices.
+         */
+        void checkIndexGroup(int natoms);
 
         //! Type of the element.
         e_selelem_t                         type;

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index 8cfcae13c6e8ce846aa954aa68c2629698461da6..7a22779ef2f0037bfc97ffa7394e66dce4fe94b4 100644 (file)
--- a/src/gromacs/selection/tests/selectioncollection.cpp
+++ b/src/gromacs/selection/tests/selectioncollection.cpp
@@ -510,6 +510,27 @@ TEST_F(SelectionCollectionTest, HandlesUnsortedGroupReferenceDelayed)
     // EXPECT_THROW_GMX(sc_.compile(), gmx::APIError);
 }
 
+TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroup)
+{
+    ASSERT_NO_THROW_GMX(sc_.setTopology(NULL, 5));
+    ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx"));
+    EXPECT_THROW_GMX(sc_.parseFromString("group \"GrpB\""), gmx::InconsistentInputError);
+}
+
+TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroupDelayed)
+{
+    ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx"));
+    ASSERT_NO_THROW_GMX(sc_.parseFromString("group \"GrpB\""));
+    EXPECT_THROW_GMX(sc_.setTopology(NULL, 5), gmx::InconsistentInputError);
+}
+
+TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroupDelayed2)
+{
+    ASSERT_NO_THROW_GMX(sc_.setTopology(NULL, 5));
+    ASSERT_NO_THROW_GMX(sc_.parseFromString("group \"GrpB\""));
+    EXPECT_THROW_GMX(loadIndexGroups("simple.ndx"), gmx::InconsistentInputError);
+}
+
 TEST_F(SelectionCollectionTest, RecoversFromMissingMoleculeInfo)
 {
     ASSERT_NO_THROW_GMX(sc_.parseFromString("molindex 1 to 5"));

diff --git a/src/gromacs/trajectoryanalysis/runnercommon.cpp b/src/gromacs/trajectoryanalysis/runnercommon.cpp
index 70a0efd2a43da0f8412f5589593b3b46da2b5907..d61ab99a32e5a11b00ef31994b11ec445236605c 100644 (file)
--- a/src/gromacs/trajectoryanalysis/runnercommon.cpp
+++ b/src/gromacs/trajectoryanalysis/runnercommon.cpp
@@ -373,17 +373,6 @@ TrajectoryAnalysisRunnerCommon::initFirstFrame()
                                                      "Trajectory (%d atoms) does not match topology (%d atoms)",
                                                      impl_->fr->natoms, top.topology()->atoms.nr)));
         }
-        // TODO: Check index groups if they have been initialized based on the topology.
-        /*
-           if (top)
-           {
-            for (int i = 0; i < impl_->sel->nr(); ++i)
-            {
-                gmx_ana_index_check(impl_->sel->sel(i)->indexGroup(),
-                                    impl_->fr->natoms);
-            }
-           }
-         */
     }
     else
     {