diff --git a/Project.toml b/Project.toml index ac7629b..213cdf3 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "SciMLTesting" uuid = "09d9d899-5365-40a9-917a-5f67fddea283" authors = ["SciML"] -version = "2.3.0" +version = "2.4.0" [deps] Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595" diff --git a/README.md b/README.md index e3dda00..a716849 100644 --- a/README.md +++ b/README.md @@ -50,9 +50,10 @@ test = ["Test", "SciMLTesting", ...] | `current_group(; env = "GROUP", default = "All")` | Read the test-group env var, defaulting to `"All"` (empty string also normalizes to the default). | | `activate_group_env(group_dir; parent, develop, instantiate, develop_sources)` | `Pkg.activate` a per-group `Project.toml`, `develop` the parent package(s) by path, backport `[sources]`, `instantiate`. | | `develop_sources!(group_dir; parent)` | On Julia < 1.11, `Pkg.develop` the env's `[sources]` path graph (recursively); a no-op on 1.11+. | -| `run_qa(pkg; Aqua, JET, ExplicitImports, aqua, jet, explicit_imports, api_docs, aqua_broken, jet_broken, ei_broken, ...)` | Run the standard Aqua/JET/ExplicitImports QA body, plus the public-API documentation check. Aqua and ExplicitImports run by default; `using JET` registers JET via its weakdep extension and turns the JET check on. `api_docs` is **on by default** and runs `run_api_docs`. The `*_broken` kwargs mark known-broken findings as `@test_broken`. | -| `run_api_docs(pkg; docstrings = true, rendered = true, docs_src, ignore, rendered_ignore, docstrings_broken, rendered_broken)` | Assert every exported/`public` name of `pkg` has a docstring and is rendered in a `@docs` block under `docs/src`. The shared replacement for per-repo `test/QA/public_api_docs.jl` files. | +| `run_qa(pkg; Aqua, JET, ExplicitImports, aqua, jet, explicit_imports, api_docs, check_reexports, aqua_broken, jet_broken, ei_broken, ...)` | Run the standard Aqua/JET/ExplicitImports QA body, plus the public-API documentation check. Aqua and ExplicitImports run by default; `using JET` registers JET via its weakdep extension and turns the JET check on. `api_docs` is **on by default** and runs `run_api_docs`. `check_reexports` is opt-in and rejects public dependency bindings except `reexports_allow`. The `*_broken` kwargs mark known-broken findings as `@test_broken`. | +| `run_api_docs(pkg; docstrings = true, rendered = true, docs_src, ignore, rendered_ignore, docstrings_broken, rendered_broken)` | Assert every exported/`public` name of `pkg` has a docstring and every locally rendered name appears in a `@docs` block under `docs/src`. Re-exported dependency modules inherit their defining package's rendering. | | `public_api_names(pkg)` | The sorted public API of `pkg` (exported names, plus `public` names on Julia ≥ 1.11), with the module's own name dropped. | +| `public_reexports(pkg; allow = ())` | Public names imported from outside the package module hierarchy, plus aliases with reflectable external module ownership, excluding intentional names in `allow`. | | `detect_sublibrary_group(group, lib_dir; default_group = "Core")` | Map a `GROUP` value to a `(sublibrary, test_group)` pair for a monorepo. | All are documented with full docstrings; `?run_tests` etc. at the REPL. @@ -330,7 +331,7 @@ run_qa(MyPackage) run_qa(MyPackage; api_docs_kwargs = (; rendered = false)) # Standalone (outside run_qa), e.g. as its own QA file: -run_api_docs(MyPackage) # every public name has a docstring and is rendered +run_api_docs(MyPackage) # every public name is documented run_api_docs(MyPackage; rendered = false) # docstrings only ``` @@ -338,10 +339,10 @@ run_api_docs(MyPackage; rendered = false) # docstrings only docstring. A re-exported name documented in its defining package counts as documented (the check follows the binding), so you are not forced to redocument dependency re-exports. - * **`rendered`** (default `true`) — every public name appears in a - ` ```@docs ` block under `docs_src` (defaults to `/docs/src`). A - ` ```@autodocs ` block satisfies it wholesale. Packages without a resolvable local - manual must explicitly pass `rendered = false`. + * **`rendered`** (default `true`) — every public name except re-exported dependency + modules appears in a ` ```@docs ` block under `docs_src` (defaults to + `/docs/src`). A ` ```@autodocs ` block satisfies it wholesale. Packages + without a resolvable local manual must explicitly pass `rendered = false`. * **`ignore` / `rendered_ignore`** — names to exclude (e.g. an un-documentable re-export), with a comment pointing at the tracking issue. * **`docstrings_broken` / `rendered_broken`** — mark the check `@test_broken` for a @@ -350,6 +351,21 @@ run_api_docs(MyPackage; rendered = false) # docstrings only On the Julia 1.10 LTS `public_api_names` returns only the exported names (the `public` keyword is 1.11+), so no per-repo `if VERSION` guards are needed. +### Public reexport audit (`check_reexports = true`) + +The reexport audit is opt-in so adopting this SciMLTesting release does not immediately +break existing facade packages. Enable it while cleaning each package's public surface: + +```julia +run_qa(MyPackage; check_reexports = true) +run_qa(MyFacade; check_reexports = true, reexports_allow = (:solve, :remake)) +``` + +It detects imported functions, types, modules, scalar constants, macros, and operators. +It also detects local aliases when their values expose module ownership, such as +ordinary functions, declared types, and modules. Keep `reexports_allow` limited to +deliberate facade API; ordinary packages should use qualified dependency names instead. + ### Known-broken findings (`aqua_broken`, `jet_broken`, `ei_broken`) When converting a hand-rolled `qa.jl` to `run_qa` would otherwise re-red a repo that diff --git a/src/SciMLTesting.jl b/src/SciMLTesting.jl index 47a263e..eb3d6de 100644 --- a/src/SciMLTesting.jl +++ b/src/SciMLTesting.jl @@ -56,7 +56,7 @@ import REPL export current_group, activate_group_env, run_qa, run_explicit_imports, detect_sublibrary_group, develop_sources!, run_tests, run_everything, read_test_groups, - with_clean_persistent_tasks_sources, run_api_docs, public_api_names + with_clean_persistent_tasks_sources, run_api_docs, public_api_names, public_reexports # Group names that are never sublibraries and never named functional groups: the # routing keywords `run_tests` and `detect_sublibrary_group` reserve. @@ -609,10 +609,10 @@ end """ run_qa(pkg; Aqua = Aqua, JET = ..., ExplicitImports = ExplicitImports, aqua = Aqua !== nothing, jet = JET !== nothing, - explicit_imports = true, api_docs = true, + explicit_imports = true, api_docs = true, check_reexports = false, clean_sources = true, aqua_kwargs = (;), jet_kwargs = (; target_modules = (pkg,), mode = :typo), - ei_kwargs = (;), api_docs_kwargs = (;), + ei_kwargs = (;), api_docs_kwargs = (;), reexports_allow = (), aqua_broken = (), jet_broken = false, ei_broken = (), testset = "Quality Assurance") @@ -633,8 +633,10 @@ Each tool runs if it is both available and enabled: * `ExplicitImports` + `explicit_imports` ⇒ ExplicitImports' standard + public-API checks (see [`run_explicit_imports`](@ref)). * `api_docs` ⇒ the public-API documentation check (see [`run_api_docs`](@ref)): every - exported/`public` name has a docstring and is rendered in the manual unless - `api_docs_kwargs` explicitly sets `rendered = false`. + exported/`public` name has a docstring and every locally rendered name appears in + the manual unless `api_docs_kwargs` explicitly sets `rendered = false`. + * `check_reexports` ⇒ fail when the package publicly exposes a binding or alias + owned outside its module hierarchy, except names listed in `reexports_allow`. Enable-flag defaults: `aqua` defaults to `Aqua !== nothing` (on — Aqua is always available; pass `Aqua = nothing` or `aqua = false` to skip it), and `jet` defaults to @@ -723,6 +725,10 @@ run_qa(MyPackage) using SciMLTesting, MyPackage run_qa(MyPackage) +# Reject accidental public reexports. Facade packages can allow intentional names: +run_qa(MyPackage; check_reexports = true) +run_qa(MyFacade; check_reexports = true, reexports_allow = (:solve, :remake)) + # Explicitly threading the modules in still works (backward-compatible): using SciMLTesting, Aqua, JET run_qa(MyPackage; Aqua = Aqua, JET = JET, jet = true) @@ -745,6 +751,8 @@ function run_qa( jet::Bool = JET !== nothing, explicit_imports::Bool = true, api_docs::Bool = true, + check_reexports::Bool = false, + reexports_allow = (), clean_sources::Bool = true, aqua_kwargs = (;), jet_kwargs = (; target_modules = (pkg,), mode = :typo), @@ -793,6 +801,14 @@ function run_qa( end explicit_imports && run_explicit_imports(pkg, ExplicitImports; ei_kwargs, ei_broken) api_docs && run_api_docs(pkg; api_docs_kwargs...) + if check_reexports + reexported = public_reexports(pkg; allow = reexports_allow) + @testset "No unapproved public reexports" begin + @test isempty(reexported) + isempty(reexported) || + @info "run_qa: unapproved public reexports" pkg reexported + end + end end return nothing end @@ -878,7 +894,7 @@ included when they are re-exported with `export`, since `names` reports them. ```julia julia> public_api_names(SciMLTesting) -11-element Vector{Symbol}: +13-element Vector{Symbol}: :activate_group_env ⋮ ``` @@ -888,6 +904,57 @@ function public_api_names(pkg::Module) return sort!(api) end +""" + public_reexports(pkg; allow = ()) -> Vector{Symbol} + +Return public names imported from, or aliased to API owned outside, `pkg`'s module +hierarchy. This is an opt-in QA audit: facade packages may allow deliberate reexports, +while ordinary packages should expose only their own API. + +`allow` is a collection of public names that are intentional reexports. Imported +bindings are detected regardless of value kind. Local aliases are also detected when +their values expose module ownership, including ordinary functions, declared types, +and modules. Julia does not retain source-binding provenance for assigned scalar or +enum values, callable objects, or constructed `Union` types; import those bindings +directly when they must be covered by this audit. + +# Examples + +```julia +public_reexports(MyPackage) +public_reexports(MyFacade; allow = (:solve, :remake)) +``` +""" +function public_reexports(pkg::Module; allow = ()) + allowed = Set(Symbol.(allow)) + reexported = filter(public_api_names(pkg)) do name + name in allowed && return false + isdefined(pkg, name) || return false + binding_owner = try + which(pkg, name) + catch + pkg + end + !_is_within_module(binding_owner, pkg) && return true + value_owner = try + parentmodule(getfield(pkg, name)) + catch + pkg + end + return !_is_within_module(value_owner, pkg) + end + return sort!(reexported) +end + +function _is_within_module(candidate::Module, root::Module) + while candidate !== root + parent = parentmodule(candidate) + parent === candidate && return false + candidate = parent + end + return true +end + # Whether `pkg.name` resolves to a docstring. Uses `Base.Docs.doc` on the *binding* # (not the object), so a re-exported name that is documented in the module that # actually defines it counts as documented here too (the binding follows the import @@ -900,6 +967,15 @@ function _has_docstring(pkg::Module, name::Symbol) return !occursin("No documentation found", doc) end +# A re-exported module inherits its defining package's module documentation. +# Re-exported functions and types still require an explicit local API entry. +function _requires_local_rendering(pkg::Module, name::Symbol) + isdefined(pkg, name) || return true + value = getfield(pkg, name) + value isa Module || return true + return _is_within_module(value, pkg) +end + # The bare name referenced by one line inside a ```@docs``` fenced block. A `@docs` # entry is a name, optionally module-qualified and/or with a call signature, e.g. # `foo`, `MyPkg.foo`, `foo(x::Int)`, `MyPkg.@mac`. Take the first whitespace token, @@ -979,8 +1055,9 @@ Two checks, each its own nested `@testset`: not forced to redocument names it re-exports from a dependency. * **rendered** (`rendered = true`, on by default): every public API name appears inside a ```` ```@docs ```` block somewhere under `docs_src`, so it is rendered in the - manual. Packages without a resolvable local manual must explicitly set - `rendered = false`. If any + manual. Re-exported modules inherit the defining package's rendered module + documentation, while re-exported functions and types still require a local entry. + Packages without a resolvable local manual must explicitly set `rendered = false`. If any ```` ```@autodocs ```` block is present the check passes wholesale — `@autodocs` renders whole modules, so anything with a docstring is already rendered. @@ -1042,8 +1119,12 @@ function run_api_docs( if rendered (rendered_names, autodocs) = _rendered_doc_names(docs_src) skip = Set{Symbol}(Symbol.(rendered_ignore)) - unrendered = autodocs ? Symbol[] : - sort!(filter(n -> !(n in skip) && !(n in rendered_names), api)) + unrendered = autodocs ? Symbol[] : sort!( + filter( + n -> _requires_local_rendering(pkg, n) && !(n in skip) && !(n in rendered_names), + api, + ) + ) @testset "public API is rendered in docs" begin rendered_broken ? (@test_broken isempty(unrendered)) : (@test isempty(unrendered)) diff --git a/test/runtests.jl b/test/runtests.jl index de66cc5..950dd81 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -94,6 +94,86 @@ module ApiFixture undocumented_public(x) = x end +module ModuleReexportFixture + import SciMLTesting + export SciMLTesting +end + +module FunctionReexportFixture + import SciMLTesting: run_qa + export run_qa +end + +module ReexportOwnerFixture + export owned_function, OwnedType, OwnedModule, owned_scalar, @owned_macro + owned_function() = nothing + struct OwnedType end + module OwnedModule end + const owned_scalar = 42 + macro owned_macro() + return nothing + end +end + +module ComprehensiveReexportFixture + import ..ReexportOwnerFixture: owned_function, OwnedType, OwnedModule, owned_scalar + export owned_function, OwnedType, OwnedModule, owned_scalar, local_function, + LocalType, local_scalar + local_function() = nothing + struct LocalType end + const local_scalar = 7 +end + +module AliasReexportFixture + import ..ReexportOwnerFixture + const aliased_function = ReexportOwnerFixture.owned_function + const AliasedType = ReexportOwnerFixture.OwnedType + const AliasedModule = ReexportOwnerFixture.OwnedModule + export aliased_function, AliasedType, AliasedModule +end + +module MacroReexportFixture + import ..ReexportOwnerFixture: @owned_macro + export @owned_macro +end + +module OperatorReexportFixture + import Base: + + export + +end + +module UndefinedExportFixture + export undefined_name +end + +module PublicOnlyReexportFixture + import ..ReexportOwnerFixture: owned_function + @static if VERSION >= v"1.11" + eval(Expr(:public, :owned_function)) + end +end + +module NestedOwnerFixture + module Internal + nested_function() = nothing + struct NestedType end + module NestedModule end + const nested_scalar = 11 + var"-->"(x, y) = x + macro nested_macro() + return nothing + end + end + import .Internal: nested_function, NestedType, NestedModule, nested_scalar, -->, + @nested_macro + export nested_function, NestedType, NestedModule, nested_scalar, -->, @nested_macro +end + +module LocalModuleFixture + module LocalSubmodule end + export LocalSubmodule +end + # A minimal AbstractTestSet that just collects every recorded result (including # nested testsets) and NEVER throws on finish. Wrapping a run_qa call in one lets a # test inspect the Broken/Pass/Fail/Error counts a broken-marker produced without @@ -838,8 +918,10 @@ end # SciMLTesting's own exported API is exactly what `export` lists (it declares # no `public` names), independent of Julia version. st = public_api_names(SciMLTesting) + @test length(st) == 13 @test :run_api_docs in st && :run_qa in st && :run_tests in st @test :run_everything in st + @test :public_reexports in st @test !(:SciMLTesting in st) end @@ -853,6 +935,34 @@ end @test SciMLTesting._doc_entry_name("MyPkg.@mac") == Symbol("@mac") end + @testset "public_reexports" begin + @test public_reexports(ModuleReexportFixture) == [:SciMLTesting] + @test public_reexports(FunctionReexportFixture) == [:run_qa] + @test public_reexports(ComprehensiveReexportFixture) == + [:OwnedModule, :OwnedType, :owned_function, :owned_scalar] + @test public_reexports(AliasReexportFixture) == + [:AliasedModule, :AliasedType, :aliased_function] + @test public_reexports(MacroReexportFixture) == [Symbol("@owned_macro")] + @test isempty(public_reexports(MacroReexportFixture; allow = (Symbol("@owned_macro"),))) + @test public_reexports(OperatorReexportFixture) == [:+] + @test isempty(public_reexports(OperatorReexportFixture; allow = (:+,))) + @test isempty(public_reexports(UndefinedExportFixture)) + @test isempty(public_reexports(LocalModuleFixture)) + @test isempty(public_reexports(NestedOwnerFixture)) + if VERSION >= v"1.11" + @test public_reexports(PublicOnlyReexportFixture) == [:owned_function] + else + @test isempty(public_reexports(PublicOnlyReexportFixture)) + end + @test public_reexports( + ComprehensiveReexportFixture; + allow = (:OwnedModule, :OwnedType, :owned_function, :owned_scalar), + ) == Symbol[] + @test public_reexports( + ComprehensiveReexportFixture; allow = (:owned_scalar,), + ) == [:OwnedModule, :OwnedType, :owned_function] + end + @testset "_rendered_doc_names" begin # A docs/src tree with a @docs block (module-qualified + signature entries) in # one file and a plain-prose file with no block in another. @@ -1018,6 +1128,31 @@ end end @test c[:fail] == 0 && c[:pass] == 1 + # A re-exported module inherits its defining package's module documentation. + rroot = mktempdir() + rsrc = joinpath(rroot, "src"); mkpath(rsrc) + c = counts_of() do + run_api_docs(ModuleReexportFixture; docstrings = false, docs_src = rsrc) + end + @test c[:fail] == 0 && c[:error] == 0 && c[:pass] == 1 + @test :SciMLTesting in public_api_names(ModuleReexportFixture) + @test !SciMLTesting._requires_local_rendering(ModuleReexportFixture, :SciMLTesting) + + c = counts_of() do + run_api_docs(FunctionReexportFixture; docstrings = false, docs_src = rsrc) + end + @test c[:fail] == 1 && c[:error] == 0 && c[:pass] == 0 + @test :run_qa in public_api_names(FunctionReexportFixture) + @test SciMLTesting._requires_local_rendering(FunctionReexportFixture, :run_qa) + @test SciMLTesting._requires_local_rendering(ComprehensiveReexportFixture, :OwnedType) + @test SciMLTesting._requires_local_rendering(NestedOwnerFixture, :NestedModule) + + # A package-owned submodule remains part of this package's rendered manual. + c = counts_of() do + run_api_docs(LocalModuleFixture; docstrings = false, docs_src = rsrc) + end + @test c[:fail] == 1 + # An @autodocs block satisfies the rendered check wholesale (no per-name list). adroot = mktempdir() asrc = joinpath(adroot, "src"); mkpath(asrc) @@ -1099,6 +1234,59 @@ end @test c[:pass] == 0 && c[:fail] == 0 && c[:error] == 0 && c[:broken] == 0 end + @testset "run_qa public reexport integration (opt in)" begin + function count_results(ts) + counts = Dict(:pass => 0, :fail => 0, :error => 0, :broken => 0) + for r in ts.results + if r isa Test.Pass + counts[:pass] += 1 + elseif r isa Test.Fail + counts[:fail] += 1 + elseif r isa Test.Error + counts[:error] += 1 + elseif r isa Test.Broken + counts[:broken] += 1 + elseif r isa Test.AbstractTestSet + sub = count_results(r) + for k in keys(counts) + counts[k] += sub[k] + end + end + end + return counts + end + counts_of(body) = count_results( + @testset ProbeTestSet "probe" begin + body() + end + ) + qa_kwargs = (; + Aqua = nothing, JET = nothing, ExplicitImports = nothing, + explicit_imports = false, api_docs = false, + ) + + # Compatibility: the audit is off unless a package explicitly enables it. + c = counts_of() do + run_qa(ComprehensiveReexportFixture; qa_kwargs...) + end + @test c[:pass] == 0 && c[:fail] == 0 && c[:error] == 0 + + c = counts_of() do + run_qa(ComprehensiveReexportFixture; qa_kwargs..., check_reexports = true) + end + @test c[:fail] == 1 && c[:error] == 0 + + c = counts_of() do + run_qa( + ComprehensiveReexportFixture; + qa_kwargs..., + check_reexports = true, + reexports_allow = (:OwnedModule, :OwnedType, :owned_function, :owned_scalar), + ) + end + @test c[:pass] == 1 && c[:fail] == 0 && c[:error] == 0 + end + @testset "run_tests routing" begin # A scratch test/runtests-like layout with body files we can detect having # run via marker files (so we can assert routing without nested Pkg.test).