Enumerate the non-public names ModelingToolkit reaches for - #4883
Conversation
|
|
||
| eqs = collect(equations(state)) | ||
| fullvars_to_idx = Dict{SymbolicT, Int}(Iterators.map(reverse, enumerate(fullvars))) | ||
| fullvars_to_idx = Dict{SymbolicT, Int}(v => i for (i, v) in enumerate(fullvars)) |
There was a problem hiding this comment.
Why is Iterators.map a problem? This is intentional, generators build a closure and Iterators.map doesn't.
There was a problem hiding this comment.
It should be moved to being an allowed.
There was a problem hiding this comment.
You're right, reverted — Iterators.map is back. I'd taken it as reducible without checking that the rewrite was behaviour-preserving, and it isn't: the generator allocates a closure.
It shows up as a finding only on the lts half of the QA matrix, because map was not public in Base.Iterators until 1.12:
julia-1.10 -e 'println(:map in names(Base.Iterators))' # false
julia-1.11 -e 'println(Base.ispublic(Base.Iterators, :map))' # false
julia-1.12 -e 'println(Base.ispublic(Base.Iterators, :map))' # true
It is now listed in NONPUBLIC_QUALIFIED_ACCESSES with that reason, per @ChrisRackauckas.
There was a problem hiding this comment.
Done — Iterators.map is in NONPUBLIC_QUALIFIED_ACCESSES now, and src/systems/alias_elimination.jl is back to the original Iterators.map line. The entry carries the reason it exists at all: map only became public in Base.Iterators in Julia 1.12, so it is a finding on lts and inert on 1.
`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>
`all_explicit_imports_are_public` and `all_qualified_accesses_are_public` both failed. With the `*_via_owners` checks fixed, every remaining finding is a name already reached through the module that defines it: there is no other module to import it from, so the only real fixes are for the owner to declare the name `public` (with a docstring and a rendered docs entry) or for ModelingToolkit to stop needing it. Three findings were reducible, and are fixed here rather than listed: six `ModelingToolkit.`-qualified references to ModelingToolkit's own `inputs_to_parameters!`, `alias_elimination!` and `MTKTearing` alias, where every neighbouring call site already spells them unqualified. The rest are enumerated in `NONPUBLIC_EXPLICIT_IMPORTS` and `NONPUBLIC_QUALIFIED_ACCESSES` in `test/qa/qa.jl`, grouped by owner with the reason each group is currently irreducible. Being exhaustive is the point: reaching for a *new* internal now fails the QA lane instead of sliding in unnoticed. The lists are larger than the reported finding counts because ExplicitImports stops at the first failing module, so the `StructuralTransformations` submodule's imports only surfaced once `ModelingToolkit` itself was clean. `Iterators.map` is listed rather than rewritten: it is deliberate in alias elimination because a generator would build a closure, and it only became `public` in Julia 1.12, so it is a finding on the `lts` half of the QA matrix and not on the `1` half. Tracked upstream in SciML#4882. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
83ef1a2 to
b2692a1
Compare
|
Rebased onto Review comments — New since the last revision — the rebase surfaced three findings master had grown in the meantime, all self-qualified accesses to ModelingToolkit's own names ( Verification is now a real before/after on the same Julia (1.12.6) and the same master commit, rather than the 1.11 run in the original description. Two things I did not fold in, both pre-existing on master:
The QA CI failure on the previous head was infrastructure, not a test failure: "The self-hosted runner lost communication with the server." |
CI on
|
| group | lts | 1 | pre |
|---|---|---|---|
| InterfaceI | ✅ | ✅ | ✅ |
| InterfaceII | ✅ | ✅ | ✅ |
| Initialization | ✅ | ✅ | ✅ |
| SymbolicIndexingInterface | ✅ | ✅ | ✅ |
| Extensions | ✅ | ✅ | ✅ |
| FMI | ✅ | ✅ | ✅ |
| Downstream | ✅ | ✅ | ✅ |
| Optimization | ✅ | ✅ | cancelled |
Plus Downgrade / InterfaceI, downgrade-sublibraries / test (lib/SciCompDSL), Spell Check, runic-suggestions, and every downstream job except the two noted below.
The red checks, and why none of them are this PR
Each of these was checked against master (453c45c08e) or against the previous head:
| check | this PR | master | what it is |
|---|---|---|---|
tests / QA (julia 1) |
❌ | ❌ | Not a test failure — the self-hosted runner is killed ~49 min in ("The self-hosted runner lost communication with the server."). Same on 2c31d86323 and 82dee851d6. Filed as #4904. |
Runic / Runic Format Check |
❌ | ❌ | lib/ModelingToolkitBase/src/problems/initializationproblem.jl:172 is missing a return, from b77c4757f5. Running Runic over every tracked .jl at this branch's HEAD reports that one diff and nothing else. |
Catalyst/Modeling/1 |
❌ | ❌ | pre-existing |
ModelingToolkitStandardLibrary.jl/Core/1/ |
❌ | ❌ | pre-existing |
SciMLBase.jl/SymbolicIndexingInterface/1/ |
❌ | ❌ | pre-existing (and SciMLBase.jl/Downstream/1/, which was red on the previous head, is green here) |
downgrade-sublibraries / test (lib/ModelingToolkitBase) |
❌ | ❌ | pre-existing |
Benchmarks (1) / (lts) |
❌ / cancelled | n/a | Failed to delete old caches; also red on the previous head |
So the QA lane — the one this PR is actually about — has no green CI result to point at, on this branch or on master, because the job never finishes. The before/after in the description is a local run of the same test/qa/qa.jl on Julia 1.12.6 against the same master commit, which is the best evidence available until #4904 is sorted.
Please ignore until reviewed by @ChrisRackauckas.
Rebased onto current master (
453c45c08e). #4880 is merged; this branch is now stacked only on #4881 — review that one first, this branch contains its commit.Problem
all_explicit_imports_are_publicandall_qualified_accesses_are_publicboth fail on master. With #4880 in place the*_via_ownerschecks pass on Julia 1.12, which means every one of these names is already reached through the module that defines it — there is no other module to import it from. The only real fixes are for the owner to declare the namepublic(with a docstring and a rendered docs entry, since public and documented move together) or for ModelingToolkit to stop needing the name.Change
Three findings were reducible, and are fixed rather than listed — six
ModelingToolkit.-qualified references to ModelingToolkit's owninputs_to_parameters!,alias_elimination!andMTKTearingalias, where every neighbouring call site already spells them unqualified:The rest are enumerated in
NONPUBLIC_EXPLICIT_IMPORTS(76) andNONPUBLIC_QUALIFIED_ACCESSES(96) intest/qa/qa.jl, grouped by owner, each group carrying the reason it is currently irreducible:COMMON_*sentinels, docstring templates). They are internal to the pair, not to ModelingToolkitBase alone, so declaring thempublicwould advertise them to end users — the opposite of what they are.TearingState, which ModelingToolkit alreadyexports and therefore already exposes as its own public API.BasicSymbolicrepresentation ModelingToolkit compiles against, the codegen targets, and the IR-walking helpers used by alias elimination and SCC construction.Base.RefValue, which has no public spelling at all.Being exhaustive is the point: reaching for a new internal now fails the QA lane instead of sliding in unnoticed. Tracked upstream in #4882, which lists every name and a suggested order of attack.
The lists are larger than the reported finding counts because ExplicitImports stops at the first failing module — the
StructuralTransformationssubmodule's imports only surfaced onceModelingToolkititself was clean.On
Iterators.mapReverted. The previous revision rewrote it as a generator; @AayushSabharwal pointed out that is a behaviour change, since a generator builds a closure and
Iterators.mapdoes not. It is listed instead, per @ChrisRackauckas.It is a finding at all only because of the Julia version:
so it is a finding on the
ltshalf of the QA matrix and not on the1half. The entry is inert on 1.12.Verification
test/qa/qa.jl, Julia 1.12.6, same master commit (453c45c08e) on both sides.Before — unmodified master:
After — this branch (which includes #4881):
all_qualified_accesses_are_publicandall_explicit_imports_are_publicgo error → pass;No unapproved public reexportsgoes fail → pass (that one is #4881's). Every Aqua testset passes identically on both sides.Also run:
Runic.main(["--check"])andtyposover the three changed files — both clean.Not verified
ltshalf of the QA matrix.test/test_groups.tomlruns QA onltsand1. On Julia 1.10 the run never reaches the two checks this PR is about, becauseall_explicit_imports_via_ownersandall_qualified_accesses_via_ownerserror first, on findings that do not appear on 1.12 —Base.whichresolves re-exported bindings differently before the 1.12 binding-partition change. The first few areSymbolics.unwrap/Symbolics.scalarize(ownerSymbolicUtils),ModelingToolkit.t_nounits/D_nounitsin the three OrdinaryDiffEq extensions (ownerModelingToolkitBase), andModelingToolkit.ImperativeAffect/SymbolicDiscreteCallbackin MTKFMIExt (ownerModelingToolkitBase), and ExplicitImports reports one module at a time so the chain keeps going. That is Import public API from its owning module, not a re-exporter #4880's subject on a Julia version Import public API from its owning module, not a re-exporter #4880 was not verified against, not this PR's, and it is pre-existing on master either way. Filed as QA: the ExplicitImports *_via_owners checks still fail on the lts (Julia 1.10) half of the matrix #4903 rather than folded in here.julia pre. Not run locally.tests / QA (julia 1, ...)fails on this PR, but it fails identically on master and it is not a test failure: the self-hosted runner is killed ~50 minutes in ("The self-hosted runner lost communication with the server."). Same on2c31d86323,82dee851d6and453c45c08e. Filed as CI: the root QA job never finishes — self-hosted runner dies ~50min in, and the declared lts QA job is never created #4904. The local runs above are therefore the only evidence available for this PR; there is no green CI QA result to point at, for this branch or for master.Still red on master, untouched here
Confirmed identical on master and on this branch, in the "before"/"after" runs above:
no_implicit_importsandno_stale_explicit_imports— bothUnanalyzableModuleException: Module ModelingToolkit was found to be unanalyzable. Catalyst hit the same thing with an EnumX submodule and passesallow_unanalyzable.public API has docstrings— fails on[:StructuralTransformations, :UnPack], the two module bindings ModelingToolkit exposes publicly without a docstring. This is not the set listed inSYMBOLICS_OWNED_REEXPORTS; those nine are already ignored and are not what the check trips on.Runic / Runic Format Checkis red on this PR and on master, not because of this PR:lib/ModelingToolkitBase/src/problems/initializationproblem.jl:172is missing areturn, fromb77c4757f5. One line, belongs in its own PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01P6EpZyM3Fmov47669Gypbi