Skip to content

Do not modify the caller's list in amino_acid_composition - #218

Merged
levitsky merged 1 commit into
levitsky:masterfrom
dylanpulver:fix-amino-acid-composition-mutates-argument
Sep 1, 2026
Merged

Do not modify the caller's list in amino_acid_composition#218
levitsky merged 1 commit into
levitsky:masterfrom
dylanpulver:fix-amino-acid-composition-mutates-argument

Conversation

@dylanpulver

Copy link
Copy Markdown
Contributor

parser.amino_acid_composition(sequence, term_aa=True) modifies the list it is given.

With term_aa=True the terminal residues are removed with list.pop() (pyteomics/parser.py:677-678). When sequence is already a parsed list, parser.py:663 binds it without copying, so those pops hit the caller's list. The str branch and the list-of-tuples branch both build a fresh list via parse(); only this one aliases.

On released 5.0.1:

>>> s = parser.parse('PEPTIDE')
>>> parser.amino_acid_composition(s, term_aa=True)
{'ctermE': 1, 'ntermP': 1, 'E': 1, 'P': 1, 'T': 1, 'I': 1, 'D': 1}
>>> s
['E', 'P', 'T', 'I', 'D']
>>> parser.amino_acid_composition(s, term_aa=True)
{'ctermD': 1, 'ntermE': 1, 'P': 1, 'T': 1, 'I': 1}

Same input, two different answers, and two residues silently gone from the caller's list. The str path is idempotent, which is the control. The docstring documents a parsed list as valid input and says nothing about mutation.

Downstream of this: electrochem.py:218 tells users to call parser.amino_acid_composition(..., term_aa=True) and feed the result to charge/pI, and achrom.get_RCs_vary_lcp calls get_RCs repeatedly over the same input.

Fix: copy the list before popping.

Tests, three cases in ParserTest:

  • the argument is unchanged and repeated calls agree;
  • a parsed list gives the same composition as the string it came from, across show_unmodified_termini × term_aa;
  • one hand-derived expected composition for an explicit ['H-', 'P', ..., '-OH'] input. The literal was derived from the documented semantics (terminal residues relabelled ntermX/ctermX and dropped from the plain counts, terminal groups retained), not read off the implementation.

Measured with the CI command from .github/workflows/pythonpackage.yml (cd tests; find . -name 'test_*.py' -print0 | xargs -0 -n1 python), same venv both sides, Python 3.14. Exit codes identical across all 27 test files; the only difference in the whole run is test_parser.py going from 22 to 25 tests. Six files fail identically before and after (test_fasta, test_featurexml, test_mgf, test_mzid, test_mzml, test_traml), all with OSError: [Errno 22] from multiprocessing in a test_map* case — this sandbox cannot create POSIX semaphores. Unrelated to the change.

Two mutants, each with the source marker checked before measuring:

  • reverting the copy fails only the mutation test, as expected — the pre-existing list-path test builds a fresh list per call, so it structurally cannot see aliasing;
  • a naive terminal-handling rewrite that copies correctly but assumes positions 0 and -1 are always the terminal residues fails the hand-derived composition test. Worth noting: with only the two relational tests, that mutant survived one run in three, because the pre-existing test catches it only when a random length-1 sequence happens to be generated. That is why the literal test is there.

Not covered: the map()/multiprocessing tests and the PostgreSQL Unimod CI step, neither of which runs in my environment; Python versions other than 3.14; whether any third-party code depends on the current mutating behaviour.

Prepared with AI tooling assistance (Claude Opus 5, used as a coding agent in this repository).

With term_aa=True the terminal residues are removed with list.pop().
When `sequence` is a parsed list it was bound without copying, so the
caller's list lost two elements and a second call on the same list
returned a different composition.

Copy the list before popping, and cover the list input in the tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@levitsky levitsky left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@levitsky
levitsky merged commit cc9817f into levitsky:master Sep 1, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants