Skip to content

Pin ModelingToolkit's public reexport surface - #4881

Draft
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/qa-reexports
Draft

Pin ModelingToolkit's public reexport surface#4881
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:agent/qa-reexports

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Please ignore until reviewed by @ChrisRackauckas.

Stacked on #4880 — review that one first; this branch contains its commit.

Problem

On unmodified master (32a955a), run_qa's No unapproved public reexports check fails with 487 findings. ModelingToolkit exposes all of these publicly and owns none of them:

owner count how it gets there
ModelingToolkitBase 339 @reexport using ModelingToolkitBase + @import_mtkbase
Symbolics 83 @reexport using Symbolics inside ModelingToolkitBase
SymbolicUtils (incl. Rewriters, Code) 32 transitively via Symbolics
SciMLBase 19 problem/function types re-exported by ModelingToolkitBase
TermInterface 4 transitively via SymbolicUtils
UnPack 3 @reexport using UnPack
CommonSolve / JumpProcesses / BipartiteGraphs / ModelingToolkitTearing / StateSelection 1 each assorted

A blanket reexport cannot distinguish "we mean to expose this" from "a dependency happened to add a public binding", which is exactly what the audit is complaining about.

Change

  1. REEXPORTED_API in test/qa/qa.jl records the approved set, grouped by owner, each group carrying the reason it is intentional. This is the value the check exists to protect: the set is now version-controlled, so a dependency adding a new public name fails this test rather than silently widening ModelingToolkit's API, and every addition to the surface becomes a deliberate decision. Removing an entry is a breaking change.

  2. @reexport using Symbolics and @reexport using UnPack are dropped from src/ModelingToolkit.jl. ModelingToolkitBase already re-exports both and ModelingToolkit @reexports ModelingToolkitBase, so these two lines only duplicated a surface that already existed.

One entry is called out as an accidental leak rather than a deliberate export: find_solvables! is a StateSelection internal that StructuralTransformations exports but only ever calls qualified (StateSelection.find_solvables!). It is listed with a comment; dropping it is a breaking change and belongs in a major release, not a QA fix.

What this PR does not do

It does not shrink the surface. Trimming the names nobody uses (the sympy_* family, the Laplace/Groebner helpers, @makearray, print_ir, …) removes public API and therefore requires a major version bump, and I could not validate the removals without running the full ModelingToolkit + Catalyst + ModelingToolkitStandardLibrary suites. The pinned list makes that a tractable follow-up: the candidates are exactly the entries in the Symbolics/SymbolicUtils groups with no use site in docs/, test/, Catalyst.jl or ModelingToolkitStandardLibrary.jl.

Verification

Run against this branch on Julia 1.11, replicating run_qa's logic (public_reexports(pkg; allow = reexports_allow)), plus a stale-entry check that run_qa does not do:

REEXPORTED_API entries = 487
unique = 487
unapproved public reexports = 0
allow-list entries no longer on the surface = 0
Test Summary:                  | Pass  Total  Time
No unapproved public reexports |    2      2  0.0s

The reexport removal in src/ModelingToolkit.jl leaves the surface byte-identical — public_reexports(ModelingToolkit) was dumped before and after and diffed:

=== DIFF before/after ===
IDENTICAL SURFACE

Smoke test on this branch (Lorenz mtkcompile + solve(Rodas5P()), index-3 pendulum mtkcompile, linearization_function/linearize):

Test Summary: | Pass  Total   Time
smoke         |    3      3  22.3s

🤖 Generated with Claude Code

https://claude.ai/code/session_019ywCW8vbGoc9X3dUbmXyme

`run_qa`'s `No unapproved public reexports` check reported 487 findings.
`ModelingToolkit` exposes those names publicly but owns none of them: they
arrive through `@reexport using ModelingToolkitBase` (339 names), which itself
`@reexport`s Symbolics (83) and, transitively, SymbolicUtils (32) and
TermInterface (4), plus the SciMLBase problem types (19) and a handful from
CommonSolve/JumpProcesses/BipartiteGraphs/ModelingToolkitTearing/StateSelection/
UnPack. A blanket reexport cannot distinguish "we mean to expose this" from "a
dependency happened to add a public binding", so the audit flags all of them.

`REEXPORTED_API` in `test/qa/qa.jl` records the set as approved, grouped by
owner with the reason each group is intentional. Pinning it is the point: it is
version-controlled, so a dependency adding a new public name now fails this test
instead of silently widening ModelingToolkit's API surface.

`@reexport using Symbolics` and `@reexport using UnPack` are dropped from
`src/ModelingToolkit.jl`: ModelingToolkitBase already re-exports both, and
ModelingToolkit `@reexport`s ModelingToolkitBase, so they only duplicated an
existing surface. `public_reexports(ModelingToolkit)` returns the same 487 names
before and after, verified by diffing the two lists.

One entry is an accidental leak rather than a deliberate export:
`find_solvables!` is a StateSelection internal that `StructuralTransformations`
`export`s but only ever calls qualified. It is listed with a comment; removing
it is breaking and belongs in a major release, not here.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
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