refactor(language): the front end is a package, and degree 1 lives in it - #371
Conversation
The ten modules from the file reader to the dim checker never reach forward to a consumer — no `lowering`, no `piecewise`, no `sources`, no `api`, no `relational`. That was true by habit and unenforced: hard rule 0 claims "the layers are ordered, and imports prove it", but only the lazy-import half was ever checked, so nothing stopped `expression_parser.py` from importing `lowering.py` next month. Move nine of them under `src/lpspec/language/` and enforce membership off the path, the way `relational/`, `linopy/` and `typeset/` already are. It is the mirror of hard rule 2: the engine may not see the schema or the AST, and now the language may not see what is done with the AST it produces. That is what keeps `lps.check()` a pass with no data and no plan, and a second consumer cheap rather than a second opinion. `errors.py` stays at the top level on purpose — it is the dependency-free leaf *both* fences point at (`ENGINE_MAY_IMPORT`), and moving it in would put the language's import path on the engine's list. `lowering.py`, `piecewise.py`, `sources.py` and `api.py` stay too: each is legitimately on the line between the halves, and the flat namespace is where that belongs. No behaviour change — imports and paths only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anguage Hard rule 3 says both lanes accept exactly the same language. For the first clause of the ceiling that was true of the accepted *set* and false of the refusal: the rule lived in `lowering.py`, so the eager lane kept a hand-copy of the `**` sentence that no test compared against the original, and for `x * y` it did not refuse at all — it multiplied and surfaced whatever linopy raised, with no mention of `piecewise:` and no ROADMAP pointer. Nothing about degree is relational. `docs/design/ceiling.md` already said so outright — "degree is not a property of the plan" — it was in a consumer only because that is where it was first needed. `language/degree.py` states it once, over the resolved core AST, and both lanes ask. `lowering.py` keeps refusing what is genuinely about the plan: a `shift(by=)` that is not an integer literal, a `group_sum(by=)` that is not a declared coordinate. `tests/test_degree_parity.py` pins the result the way `test_resolution_parity.py` pins scoping — both lanes refuse, in the same words. Disabling either call site fails three of its four cases. The generalisation is written down in ARCHITECTURE under "What counts as language": a rule is language iff two consumers answering it separately would be a bug. That is the test every "one implementation each" rule in the file was already applying — resolution, the closed helper set, the dim rules — and degree is the one that had escaped it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Two commits. The first re-lands #367, which merged into its own base branch rather than into
main—origin/mainstill had the flat layout, so thelanguage/package was stranded ondocs/architecture-diagram-accuracyafter that branch merged. Cherry-picked onto currentmainand re-verified; four small conflicts from #366/#368, all import lines.The second is the new work, and it answers a question the first commit exposed: what is
language/actually for? "Imports nothing forward" is a fence, not a definition — it says what may not happen, not what belongs.The definition
Not "is it syntax", not "does it run early" — would a second opinion be wrong? Every "one implementation each" rule already in
ARCHITECTURE.mdis that test applied: names resolve once because two lanes resolving separately disagreed three ways; the helper set is closed and a test proves both lanes implement exactly it; a primitive's dim rule lives indimensions.pyand lowering asks rather than deciding again.Written down under What counts as language, along with its corollary — the flat top level is for modules that are legitimately both halves (
lowering.py,piecewise.py,sources.py,api.py), which is a real category rather than a leftovers bin.The rule that failed the test
Degree 1 — the ceiling's first clause — lived in
lowering.py. Consequences, all of them real:linopy/builder.pykept a hand-copy of the**refusal sentence, with a comment saying "this lane must agree." Nothing tested that it did.x * ythe eager lane did not refuse at all. It multiplied and let linopy raise whatever linopy raises — so the user got a library's error instead of the language's, with no mention ofpiecewise:and no ROADMAP pointer.tests/test_language_boundary.pydrives the whole boundary throughlower_program, so the eager lane's degree behaviour was unasserted.Hard rule 3 says both lanes accept exactly the same language. That was true of the accepted set and false of the refusal — the weakest possible version of the claim. And nothing about
x * yis relational:docs/design/ceiling.mdalready says outright that degree is not a property of the plan. It was in a consumer because that is where it was first needed, which is how this drift always starts.What changed
language/degree.pystates it once over the resolved core AST —carries_variablepluscheck_binary, refusing the three ways degree 1 is lost (quadratic product, variable divisor, an operator the language doesn't have). Both lanes call it; neither states it. Message text is unchanged, so every existing assertion still matches.lowering.pykeeps refusing what is genuinely about the plan —shift(by=)must be an integer literal,group_sum(by=)a declared coordinate. A second opinion about those isn't a bug, it's the other lane's business. Its plan-level_has_varis gone; the shift-fill decision now asks the AST.tests/test_degree_parity.pypins it the waytest_resolution_parity.pypins scoping: both lanes refuse, in the same words, with each case recording what the eager lane used to do instead. Verified adversarially — disabling either call site fails three of its four cases.What did not move, and why
piecewise.pystays at the top level. I expected it to follow degree intolanguage/, but a link expression genuinely wants the whole subset test, not just affinity — it can containshift/group_sum, andcheck_core_subsetis what refuses a shape the engine has no plan node for. So it is a language module that asks a plan question: exactly the both-halves category. The one declared lazy import therefore stays too.Test plan
uv run pytest— 705 passed, 4 skipped, 1 xfailed (4 new)degree.check_binarydisabled inbuilder.py→ 3 of 4 parity cases fail; restored → passruff check/format --checkclean;pyrefly0 errors onsrc/mkdocs build --strictbuildslps.check()refusessum(x * x, over=g)with no linopy present🤖 Generated with Claude Code