Skip to content

Fix copy/paste bugs in duplicated conditions - #762

Open
lyskov-ai wants to merge 2 commits into
RosettaCommons:mainfrom
lyskov-ai:fix/duplicated-copy-paste-conditions
Open

lyskov-ai wants to merge 2 commits into
RosettaCommons:mainfrom
lyskov-ai:fix/duplicated-copy-paste-conditions

Conversation

@lyskov-ai

@lyskov-ai lyskov-ai commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Each condition changed here tests the same operand twice, so a sibling operand is never tested and part of the check is dead code.

Behaviour fixes

core/energy_methods/CartesianBondedEnergy.cceval_singleres_torsion_derivatives tests whether all four torsion atoms are ring atoms before handing the torsion off to cart_bonded_ring. It checked atids[2] twice and never atids[4]. The two sibling angle loops in the same file (over a Size3) correctly check atids[1..3]; this one is over a Size4.

core/chemical/rna/util.cc — in the SUGAR/SUGAR CIS block of get_base_pair_atoms, the third row of the 4×4 base-pair table is labelled aa1 == na_rad, which the adenine row above it already matches, so all four of its branches are unreachable. They belong to guanine:

  • the bodies push " H22", an atom that only RGU has — RAD has " H2 " (see RAD_n.params / RGU_n.params);
  • na_rgu is the only aa1 value missing from the chain, which otherwise covers rad, rcy, ura against all four aa2 values.

protocols/minimization_packing/DisulfideOptimizationMover.cc — the check documented as "Confirm that both are cysteine-type residues in a disulfide bond to each other" tested cys_pos[1] twice, so cys_pos[2] was never validated. The error messages below it already reference both residues.

protocols/environment/DofUnlock.cc — the environment-id half of the popped-passport check compared pass_out to itself, so an environment-id mismatch was never caught. The error message below it reports pass_->env_id() as the expected value.

protocols/pose_metric_calculators/RotamerRecovery.ccruntime_assert( pose_rots.size() == pose_rots.size() ) is always true; the neighbouring assert compares against the native, and this one should too.

protocols/floppy_tail/FloppyTailMover.ccflexible_chain appeared twice in the list of options incompatible with an explicit -in:file:movemap, so flexible_start_resnum was never rejected even though the movemap overrides it. The user-facing message had the same duplication and is corrected to match.

Behaviour-preserving removals

These duplicates are simply dead and removing them changes nothing:

  • protocols/cryst/wallpaper.cc"C211" listed twice among the tetragonal groups (the file defines exactly seven distinct groups for that setting).
  • protocols/antibody/design/CDRSeqDesignOptions.cc"DISALLOWED" listed twice.
  • protocols/simple_filters/SecretionPredictionFilter.cc and protocols/protein_interface_design/movers/SecretionOptimizationMover.cc — the dG_ins threshold comparison repeated in the same && chain, left over from extending the local-minimum test from ±1 to ±2.
  • protocols/sic_dock/xyzStripeHashPoseWithMeta.hhinit_with_pose tested PoseCoordPickMode_BB twice, making the second branch unreachable. Its body collects CA/C/CB, which matches no value of PoseCoordPickMode (N_CA_C and N_CA_C_CB both include N), so the intent behind it is not recoverable and the dead branch is removed. The surviving branch collects N/CA/C/O/CB, matching both the natom counting loop directly above it and the canonical case PoseCoordPickMode_BB in core/pose/xyzStripeHashPose.cc.

Note on diff size

The diff is small relative to the usual bundling guidance because every fix is a one-line correction and this category is exhausted: the analysis pass was run over utility/, basic/, core/, protocols/ and numeric/, and utility/, basic/ and numeric/ produced no true positives. The remaining flagged sites are either the x != x NaN idiom or genuine "different conditions, same action" branches.

Each of these tests the same operand twice in one condition or if/else
chain, so a sibling operand is never tested and part of the check is dead.

Behaviour fixes:

* CartesianBondedEnergy: the torsion ring-atom test checked atids[2]
  twice and never atids[4], so 4-atom ring torsions were not always
  handed off to cart_bonded_ring.
* chemical/rna/util: in the SUGAR/SUGAR CIS base-pair table the guanine
  row was labelled `aa1 == na_rad`, which the adenine row above already
  matches. All four guanine branches were unreachable. They push " H22",
  an atom only RGU has (RAD has " H2 "), and na_rgu was the one row
  missing from the otherwise complete 4x4 table.
* DisulfideOptimizationMover: the "both residues are disulfide bonded"
  check tested cys_pos[1] twice, so cys_pos[2] was never validated.
* DofUnlock: the environment-id half of the passport check compared
  pass_out to itself instead of to pass_, so a mismatched environment id
  was never caught.
* RotamerRecovery: runtime_assert compared pose_rots.size() to itself
  instead of to nat_rots.size().
* FloppyTailMover: flexible_chain appeared twice in the list of options
  that conflict with an explicit movemap, so flexible_start_resnum was
  never rejected. The error message had the same duplication.

Behaviour-preserving removals of dead duplicate conditions:

* cryst/wallpaper: "C211" listed twice among the tetragonal groups.
* CDRSeqDesignOptions: "DISALLOWED" listed twice.
* SecretionPredictionFilter, SecretionOptimizationMover: the dG_ins
  threshold comparison repeated in the same && chain.

Found with a new redundant-expression / branch-clone analysis pass over
utility/, basic/, core/, protocols/ and numeric/; utility/, basic/ and
numeric/ had no true positives.
xyzStripeHashPoseWithMeta::init_with_pose tested
PoseCoordPickMode_BB twice in the same if/else chain, so the second
branch could never run. Its body collects CA, C and CB, which matches no
value of PoseCoordPickMode -- N_CA_C and N_CA_C_CB both include N -- so
the intent behind it is not recoverable.

The surviving first branch is the correct one: it collects N, CA, C, O
and CB, matching both the natom counting loop above it and the
canonical case PoseCoordPickMode_BB in core/pose/xyzStripeHashPose.cc.

Removing dead code, no behaviour change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant