Skip to content

Raise ValueError when a module is assigned to more than one layer (#165)#304

Open
HarperZ9 wants to merge 1 commit into
python-grimp:mainfrom
HarperZ9:fix/duplicate-layer-modules
Open

Raise ValueError when a module is assigned to more than one layer (#165)#304
HarperZ9 wants to merge 1 commit into
python-grimp:mainfrom
HarperZ9:fix/duplicate-layer-modules

Conversation

@HarperZ9

Copy link
Copy Markdown

Fixes #165.

Problem

find_illegal_dependencies_for_layers terminates with an opaque Rust panic when the same module is assigned to more than one layer:

>>> graph.find_illegal_dependencies_for_layers(layers=("grimp.domain", "grimp.domain"))
thread '<unnamed>' panicked at src/importgraph.rs:144:63:
no entry found for key
pyo3_runtime.PanicException: no entry found for key

A module cannot sit at two different levels at once, so this is an input error that should be reported clearly rather than surfacing as a panic from the extension.

Change

Add a Python-side guard (_check_layers_do_not_share_modules) that runs after _parse_layers and raises a descriptive ValueError naming the repeated module(s) before the Rust call:

ValueError: Modules can only belong to one layer, but the following appear in more than one: 'grimp.domain'.

It works on the parsed Layer.module_tails, so it covers the simple-string, set, and Layer input forms.

Tests

  • Added TestInvalidLayers in tests/unit/application/graph/test_layers.py for the string and Layer duplicate cases.
  • pytest tests/unit tests/functional -> 828 passed, 1 skipped.
  • ruff check, ruff format --check, and mypy on the changed files pass.
  • Added a CHANGELOG.rst entry under latest.

find_illegal_dependencies_for_layers panicked with an opaque
pyo3_runtime.PanicException ("no entry found for key") when the same module
appeared in more than one layer. Add a Python-side guard that raises a
descriptive ValueError naming the repeated module(s) before the call reaches
the Rust extension, and cover it with tests.

Fixes python-grimp#165.

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

codspeed-hq Bot commented Jun 29, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 23 skipped benchmarks1


Comparing HarperZ9:fix/duplicate-layer-modules (e664104) with main (977b7f0)

Open in CodSpeed

Footnotes

  1. 23 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@HarperZ9

Copy link
Copy Markdown
Author

Heads-up on the red CI here: it is not caused by this change. main currently fails the Lint and check docs build job on two pre-existing lint issues from tool version bumps (ruff FURB110 in tests/adaptors/filesystem.py and clippy collapsible_match in rust/src/import_parsing.rs), and because that job shares the workflow with the test matrix under fail-fast, it cancels the test jobs too. #305 fixes both. Once that lands I will rebase this PR so its checks go green. The change itself is unrelated to the lint failures.

@seddonym

seddonym commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR! I'll have a look in the next few days.

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.

PanicError when duplicate layers are passed to find_illegal_dependencies

2 participants