test: add Aqua QA; migrate to JSON 1.x#4
Merged
Conversation
QA suite:
- Aqua.test_all(WasmTarget) — catches method ambiguities, unbound
type params, undefined exports, stale deps, missing compat,
piracy, persistent tasks. All 11 sub-checks pass.
- ExplicitImports individual checks — no_implicit_imports,
no_stale_explicit_imports, explicit_imports_via_owners,
qualified_accesses_via_owners, no_self_qualified_accesses
(with `optimize` ignored — it's both a public kwarg and an
exported function, the self-qualification is necessary to
disambiguate inside kwarg-shadowed scopes). We skip the two
"are_public" checks because WasmTarget intentionally reaches
into Base/Core/Compiler internals (uncompressed_ir, memoryref,
method_table, pow_body, etc.) — that's how compiling Julia
to Wasm works.
Findings fixed while wiring this up:
- Drop stale `IRStatement` and `IntrinsicMapping` exports
(neither symbol existed).
- Drop unused `Base.Experimental.@MethodTable` import at
codegen/interpreter.jl:19 — the overlay table is defined via
the qualified `Base.Experimental.@MethodTable` macro, not via
the imported name.
- Add stdlib compat entries for Dates and Test.
JSON 1.x migration:
- Bump compat to JSON = "1" (JSON 1.x is where the ecosystem is
heading; JSON 0.21 is legacy).
- `unmarshal_result` in test/utils.jl previously guarded on
`result isa Dict`, but JSON 1.x's `JSON.parse` returns
`JSON.Object <: AbstractDict`, not `Dict`. The `__bigint__`
sentinel unwrap path was silently skipped, leaving 600+ tests
failing on every CI run with assertions like
`JSON.Object("__bigint__" => "30") == 30`. Widen the guards
to `AbstractDict` so both JSON 0.21 and 1.x work.
README:
- Add Aqua QA and ExplicitImports.jl badges under the CI row.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI runners were OOM'ing in the ExplicitImports block because the
individual check_* functions each re-walk every src/ file from
scratch — ~30 KB of codegen source × 5 checks blew past the 7 GB
Ubuntu/macOS runner limit ("hosted runner lost communication with
the server" after 25 min on macOS, 55 min on Ubuntu).
test_explicit_imports(...) initializes file_analysis once at the
start and passes it to every internal test_* call, so the AST walk
runs a single time. Dropped local runtime from 13m35s → 2m33s.
Keeps the same semantic coverage as before:
- `all_explicit_imports_are_public = false` — WasmTarget reaches
into Base/Core/Compiler internals on purpose (it's a compiler)
- `all_qualified_accesses_are_public = false` — same reason
- `no_self_qualified_accesses = (; ignore=(:optimize,))` — the
optimize kwarg on compile_* shadows the exported function, so
`WasmTarget.optimize(...)` is the only way to reach it inside
those methods
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ExplicitImports emits `@warn default_param_context reached recursion
limit` once per deeply-nested type parameter it can't fully walk.
For a compiler package like WasmTarget this fires thousands of times
and each warning dumps the entire unresolved `ImplicitCursor{...}`
type — a multi-MB blob per warning. GitHub's log viewer truncates
around 4 MB with "This step has been truncated due to its large
size", which makes any real failure impossible to locate.
The previous run's log was 235 MB of exactly those ~3 KB type dumps
with the test summary buried at the bottom.
Filter just that one warning text with a lightweight AbstractLogger
wrapper. All other log messages still pass through to the user.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ExplicitImports spent 13 min on the codegen analysis locally, OOM'd Ubuntu+macOS CI runners, and dumped thousands of multi-MB recursion warnings that made logs unreadable. Not worth the operational cost for this package. The intrinsic-reach-into-Base nature of a compiler is also fundamentally at odds with ExplicitImports' "are_public" model. Reverts only the ExplicitImports wiring: - delete test/test_explicit_imports.jl - drop include from test/runtests.jl - drop ExplicitImports from [extras]/[compat]/[targets] - drop badge from README Aqua (all 11 sub-checks passing in ~6 s) and the JSON 1.x migration (the actual bug fix for the 657-test regression on main) stay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dale-Black
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Aqua.jl package-quality checks to the test suite, and migrates the package to JSON 1.x — which also fixes the 657-test regression that's currently on
mainfrom when CompatHelper merged the JSON 0.21→1 bump (PR #3).What's in
Aqua (
test/test_aqua.jl) —Aqua.test_all(WasmTarget)runs all 11 sub-checks (method ambiguities, unbound type params, undefined exports, stale deps, missing compat, piracy, persistent tasks, project_toml/test_project_toml parity). All 11 pass locally in ~6 s.JSON 1.x migration — pins
JSON = "1".test/utils.jl::unmarshal_resultguarded onresult isa Dict, but JSON 1.x'sJSON.parsereturnsJSON.Object <: AbstractDict, notDict. The__bigint__sentinel unwrap silently no-op'd, leaving 657 tests failing per OS with assertions likeJSON.Object("__bigint__" => "30") == 30. Widen guards toAbstractDict. Sanity-checked on JSON 1.5.0:30Int6430{"__bigint__":"30"}JSON.Object30{"__bigint__":"9007199254740994"}JSON.Object9007199254740994"__NaN__"StringNaNDrive-by fixes flagged by Aqua
IRStatementandIntrinsicMapping— neither symbol was ever defined.Base.Experimental.@MethodTableimport atcodegen/interpreter.jl:19(the macro is invoked via the qualified path, not the imported name).[compat]entries forDatesandTest.README
What's NOT in (intentionally dropped)
ExplicitImports.jl was wired up and working locally but removed — its AST walk over WasmTarget's codegen source took 13 min and OOM'd Ubuntu+macOS runners, and it emitted thousands of multi-MB recursion-limit warnings that made logs unreadable. The "are_public" checks also fundamentally clash with this package's nature (a compiler intentionally reaches into Base/Core/Compiler internals). Not worth the operational overhead.
Test plan