diff --git a/Project.toml b/Project.toml index 127af9a..819d110 100644 --- a/Project.toml +++ b/Project.toml @@ -5,11 +5,12 @@ version = "1.0.0-DEV" [deps] MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09" +Markdown = "d6f4376e-aef5-505a-96c1-9c027394607a" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" [compat] -julia = "1.6.7" ReTestItems = "1" +julia = "1.6.7" [extras] ReTestItems = "817f1d60-ba6b-4fd5-9520-3cf149f6a823" diff --git a/src/StructFieldParamsTesting.jl b/src/StructFieldParamsTesting.jl index c819e3a..410a53e 100644 --- a/src/StructFieldParamsTesting.jl +++ b/src/StructFieldParamsTesting.jl @@ -4,6 +4,7 @@ export test_all_structs_have_fully_specified_fields export test_all_fields_fully_specified, field_is_fully_specified using MacroTools: @capture +using Markdown: Markdown using Test include("check_struct_fields.jl") diff --git a/src/check_struct_fields.jl b/src/check_struct_fields.jl index 9bb9464..234dbf0 100644 --- a/src/check_struct_fields.jl +++ b/src/check_struct_fields.jl @@ -1,11 +1,13 @@ function test_all_fields_fully_specified(pkg::Module, struct_expr; location = nothing) (struct_name, T, fields_dict) = _extract_struct_field_types(pkg::Module, struct_expr) @testset "$struct_name" begin - for (field_name, field_type_expr) in fields_dict - check_field_type_fully_specified(pkg, struct_name, field_name, T, - field_type_expr, - report_error = true, location = location) - @test true + @testset let struct_expr=struct_expr + for (field_name, field_type_expr) in fields_dict + check_field_type_fully_specified(pkg, struct_name, field_name, T, + field_type_expr, + report_error = true, location = location) + @test true + end end end end @@ -29,7 +31,6 @@ function _extract_struct_field_types(pkg::Module, struct_expr) ) || error("Invalid struct expression: $(struct_expr)") T === nothing && (T = []) - fields_split = split_field.(fields) filter!(x -> x !== nothing, fields_split) fields_dict = Dict{Symbol, Any}(fields_split) @@ -46,24 +47,23 @@ function check_field_type_fully_specified( mod::Module, struct_name, field_name, typevars, field_type_expr; report_error, location, ) - print("TypeVars: ") - dump(typevars) - @show mod + @debug "Module:" mod + @debug "TypeVars:" typevars TypeObj = Base.eval(mod, quote $(field_type_expr) where {$(typevars...)} end) - @show TypeObj + @debug "Type:" TypeObj @assert TypeObj isa Type if isconcretetype(TypeObj) # The type is concrete, so it is fully specified. - @info "Type is concrete: $(TypeObj)" + @debug "Type is concrete: $(TypeObj)" return true end if typeof(TypeObj) == DataType # The type is a DataType, so it is fully specified. # Presumably, it is an abstract type like `Number` or `Any` - @info "Type is a DataType: $(TypeObj)" + @debug "Type is a DataType: $(TypeObj)" return true end if typeof(TypeObj) == Union @@ -77,78 +77,173 @@ function check_field_type_fully_specified( end @assert typeof(TypeObj) === UnionAll "$(TypeObj) is not a UnionAll. Got $(typeof(TypeObj))." - num_type_params = _count_unionall_parameters(TypeObj) - num_expr_args = _count_type_expr_params(field_type_expr) - # "Less than or equal to" in order to support literal values in the type expression. - # E.g.: The UnionAll `Array{<:Int, 1}` has 1 type arg but 2 params in the expression. + num_type_params = _count_unionall_free_parameters(TypeObj) + num_expr_args = _count_type_expr_params(mod, field_type_expr) + # "Less than or equal to" in order to support fully constrained parameters in the expr. + # E.g.: `Vector{T} where T<:Int` has 0 free type params but 1 param in the expression. success = num_type_params <= num_expr_args - if report_error - @assert success field_type_not_complete_message( + if !success && report_error + throw(FieldTypeIncompleteException( mod, struct_name, field_name, field_type_expr, TypeObj, num_type_params, num_expr_args, location, - ) + )) end return success end +# TODO(type-alias): What do we actually want to do for alias types? +# E.g. +# IterableResult{K} = Union{Vector{K}, BeTreeV2_Typed{K}} +# which unwraps to +# Union{Array{K, 1}, BeTreeV2_Typed{K, V, E, KNorm, VNorm, TD}} +# I think this is a reason to separate "fully-specified" from "concrete". +# And to be fully-specified this requires only exactly 1 type param (for K). +# But to be concrete it requires all 6 type params (K, V, E, KNorm, VNorm, TD). +recursive_unwrap_unionall(@nospecialize(T)) = Base.unwrap_unionall(T) +recursive_unwrap_unionall(T::UnionAll) = recursive_unwrap_unionall(Base.unwrap_unionall(T)) +recursive_unwrap_unionall(T::Union) = Union{recursive_unwrap_unionall(T.a), recursive_unwrap_unionall(T.b)} + +# Get free TypeVar names (without constraints): +# Foo{Int, X<:Integer, Y} where {X, Y} => [:X, :Y] +function type_param_names(TypeObj) + names = Symbol[] + while typeof(TypeObj) === UnionAll + push!(names, TypeObj.var.name) + TypeObj = TypeObj.body + end + return names +end + +struct FieldTypeIncompleteException <: Exception + mod::Module + struct_name::Symbol + field_name::Symbol + field_type_expr::Any + TypeObj::Type + num_type_params::Int + num_expr_args::Int + location::Union{Nothing, String} +end +function Base.showerror(io::IO, ex::FieldTypeIncompleteException) + print(io, "FieldTypeIncompleteException: ") + field_type_not_complete_message( + io, ex.mod, ex.struct_name, ex.field_name, ex.field_type_expr, ex.TypeObj, + ex.num_type_params, ex.num_expr_args, ex.location, + ) +end +function print_error(ex::FieldTypeIncompleteException) + io = IOBuffer() + field_type_not_complete_message( + io, ex.mod, ex.struct_name, ex.field_name, ex.field_type_expr, ex.TypeObj, + ex.num_type_params, ex.num_expr_args, ex.location, + ) + seekstart(io) + # Markdown.print(Markdown.parse(String(take!(io)))) + md = Markdown.parse(io) + show(stdout, MIME"text/plain"(), md) +end + +# TODO: Fix the message! +# This works for e.g. +# struct Foo x::Dict end +# struct Foo{K} x::Dict{K} end +# struct Foo x::Dict{K} where {K} end +# but not for e.g. +# struct Foo x::Dict{Int} end function field_type_not_complete_message( - mod::Module, struct_name, field_name, field_type_expr, TypeObj, + io::IO, mod::Module, struct_name, field_name, field_type_expr, TypeObj, num_type_params, num_expr_args, location, ) # io = IOBuffer() # show(IOContext(io, :compact=>false), TypeObj) # complete_type = String(take!(io)) - typename = nameof(TypeObj) - typevars = join(["T$i" for i in 1:(num_type_params - num_expr_args)], ", ") - typestr = "$(field_type_expr)" - @show typestr - if occursin("}", typestr) - typestr = replace(typestr, "}" => ", $(typevars)}") - else - typestr = "$(typestr){$(typevars)}" + typename = TypeObj isa DataType ? nameof(TypeObj) : string(TypeObj) + type_params = type_param_names(TypeObj) + @assert num_type_params <= length(type_params) + type_params = type_params[1:num_type_params] + expr_args = num_expr_args == 0 ? Symbol[] : type_params[1:num_expr_args] + complete_type = Base.unwrap_unionall(TypeObj) + # TODO(type-alias): see comment on recursive_unwrap_unionall + complete_type_recursive = recursive_unwrap_unionall(TypeObj) + s = num_type_params == 1 ? "" : "s" + print(io, """ + In struct `$(mod).$(struct_name)`, the field `$(field_name)` does not have a fully \ + specified type:\n + \t$(field_name)::$(field_type_expr)\n + """ + ) + if location != nothing + print(io, """ + Defined at line:\n + \t$(location)\n + """) + end + print(io, """ + The complete type is:\n + \t$(complete_type)\n + which expects $(num_type_params) type parameter$(s): `$(join(type_params, ", "))`.\n + """) + if string(complete_type) != string(complete_type_recursive) + print(io, """ + And which is an alias for:\n + \t$(complete_type_recursive)\n + """) end - complete_type = "$(typestr) where {$(typevars)}" - """ - In struct `$(mod).$(struct_name)`, the field `$(field_name)` does not have a fully \ - specified type: - - `$(field_name)::$(field_type_expr)` - - Defined at line: - $(location) - - The complete type is `$(complete_type)`. The current definition specifies \ - $(num_expr_args) type arguments, but the type `$(complete_type)` expects \ - $(num_type_params) type parameter(s). This means the struct's field currently has an \ - abstract type (it is type unstable), and any access to it will cause a dynamic dispatch. - - If this was a mistake, possibly caused by a change to the `$(typename)` type that \ - introduced new parameters to it, please make sure that your field `$(field_name)` is \ - fully concrete, with all parameters specified. - - If, instead, this type instability is on purpose, please fully specify the omitted \ - type parameters to silence this message. You can write that as `$(complete_type)`, or \ - possibly in a shorter alias form which this message can't always detect. (E.g. you can \ - write `Vector{T} where T` instead of `Array{T, 1} where T`.) - """ + + print(io, """ + The current definition `$(field_type_expr)` specifies \ + $(num_expr_args == 0 ? "no type parameters." : "only $(num_expr_args) type parameters: \ + `$(join(expr_args, ", "))`.") + """) + + print(io, """ + This means the `$(field_name)` field currently has an abstract type, + and any access to it is type unstable will therefore cause a dynamic dispatch. + + If this was a mistake, possibly caused by a change to the `$(typename)` type that \ + introduced new parameters to it, please make sure that your field `$(field_name)` is \ + fully concrete, with all parameters specified. + + If, instead, this type instability is on purpose, please fully specify the omitted \ + type parameters to silence this message. You can write that as `$(complete_type)`, or \ + possibly in a shorter alias form which this message can't always detect. (E.g. you can \ + write `Vector{T} where T` instead of `Array{T, 1} where T`.) + """) + return io end -function _count_unionall_parameters(TypeObj::UnionAll) +_count_unionall_free_parameters(@nospecialize(::Any)) = 0 +function _count_unionall_free_parameters(TypeObj::UnionAll) + return _count_unionall_free_parameters(Base.unwrap_unionall(TypeObj)) +end +function _count_unionall_free_parameters(TypeObj::DataType) count = 0 - while typeof(TypeObj) === UnionAll - count += 1 - TypeObj = TypeObj.body + for param in @show TypeObj.parameters + # only `TypeVars` can be free parameters, but in `T<:ConcreteType` + # don't consider `T` as a free parameter + if param isa TypeVar && !isconcretetype(param.ub) + count += 1 + end + # The parameters might themselves be `UnionAll` + count += _count_unionall_free_parameters(param) end return count end -_count_type_expr_params(s::Symbol) = 0 -function _count_type_expr_params(expr::Expr) + +_count_type_expr_params(mod::Module, @nospecialize(x)) = 0 +# If the symbol is defined in the module it is not a type var. +_count_type_expr_params(mod::Module, s::Symbol) = isdefined(mod, s) ? 0 : 1 +function _count_type_expr_params(mod::Module, expr::Expr) count = 0 - while expr.head === :where - count += length(expr.args) - 1 + if expr.head === :where expr = expr.args[1] - end - if expr.head == :curly - count += length(expr.args) - 1 + count += _count_type_expr_params(mod, expr) + elseif expr.head == :<: + count += _count_type_expr_params(mod, expr.args[1]) + elseif expr.head == :curly + for arg in Iterators.rest(expr.args, 1) + count += _count_type_expr_params(mod, arg) + end end return count end diff --git a/src/whole_module_checks.jl b/src/whole_module_checks.jl index 6a7bc77..af3c0f0 100644 --- a/src/whole_module_checks.jl +++ b/src/whole_module_checks.jl @@ -1,50 +1,65 @@ - -function test_all_structs_have_fully_specified_fields(pkg::Module) - @testset "$(pkg)" begin +function test_all_structs_have_fully_specified_fields(pkg::Module; verbose::Bool=false) + @testset "$(pkg)" verbose=verbose begin dir = pkgdir(pkg) @assert !isnothing(dir) "No file found for Module `$(pkg)`." entrypoint = joinpath(dir, "src", "$(nameof(pkg)).jl") @assert ispath(entrypoint) "Package $(pkg) source not found: $entrypoint" - - include_and_parse_file(pkg, entrypoint) + test_file(pkg, entrypoint) end end -function include_and_parse_file(pkg::Module, file::String) +function test_file(pkg::Module, filename::String) # Parse the file and call handle_parsed_expression on each expression. - contents = read(file, String) - eval_module = Module(:FullySpecifiedFieldTypesStaticTests__Evals) - line = 0 - Base.include_string(eval_module, contents, relpath(file)) do e - line = handle_parsed_expression(pkg, e, file, line) + contents = read(filename, String) + walk_string(contents, filename) do parsed, loc + handle_parsed_expression(pkg, parsed, loc) + end +end + +# Based on `Base.include_string` but with two differences: +# 1. `mapexpr` takes a second argument which is a `LineNumberNode` +# 2. we do not eval the expressions, just call `mapexpr` on them +# - this also means we do not need a `mod::Module` argument +function walk_string(mapexpr::Function, code::AbstractString, filename::AbstractString) + loc = LineNumberNode(1, Symbol(filename)) + try + ast = Meta.parseall(code; filename) + @assert Meta.isexpr(ast, :toplevel) + for ex in ast.args + if ex isa LineNumberNode + loc = ex + continue + end + mapexpr(ex, loc) + end + return nothing + catch exc + rethrow(LoadError(filename, loc.line, exc)) end end -handle_parsed_expression(::Module, ::Any, _file, _line) = nothing -handle_parsed_expression(::Module, loc::LineNumberNode, _file, _line) = loc.line -function handle_parsed_expression(pkg::Module, parsed::Expr, file, line) +handle_parsed_expression(::Module, x::Any, _loc) = nothing +function handle_parsed_expression(pkg::Module, parsed::Expr, loc) + # @show loc if parsed.head == :struct # DO THE THING - test_all_fields_fully_specified(pkg, parsed, location = "$(file):$(line)") + location = "$(loc.file):$(loc.line)" + test_all_fields_fully_specified(pkg, parsed; location) elseif parsed.head == :call && parsed.args[1] == :include # Follow includes to more files - new_file = joinpath(dirname(file), parsed.args[2]) - include_and_parse_file(pkg, new_file) + new_file = joinpath(dirname(String(loc.file)), parsed.args[2]) + return test_file(pkg, new_file) elseif parsed.head == :module modname = parsed.args[2] inner_mod = Core.eval(pkg, modname) @testset "$(inner_mod)" begin for expr in parsed.args - if expr isa LineNumberNode - line = expr.line - else - line = handle_parsed_expression(inner_mod, expr, file, line) - end + handle_parsed_expression(inner_mod, expr, loc) end end else for expr in parsed.args - line = handle_parsed_expression(pkg, expr, file, line) + handle_parsed_expression(pkg, expr, loc) end end end diff --git a/test/check_struct_fields_tests.jl b/test/check_struct_fields_tests.jl index ed03c49..712926f 100644 --- a/test/check_struct_fields_tests.jl +++ b/test/check_struct_fields_tests.jl @@ -1,4 +1,3 @@ - @testitem "check field tests" begin # Concrete DataType @@ -17,6 +16,17 @@ :(struct S x::Vector{Int} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S x::Vector{Pair{Dict{Int,String},Set{Bool}}} end), + :x, + ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Pair{Dict{Bool}, String} end), + :x, + ) + # Fully specified field type (DataType) @test field_is_fully_specified( @__MODULE__, @@ -28,6 +38,26 @@ :(mutable struct S{T<:Int} x::Vector{T} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T<:Int} x::Dict{T,Int} end), + :x, + ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Vector{<:T} end), + :x, + ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Vector{<:T} where {Integer<:T<:Number} end), + :x, + ) + @test field_is_fully_specified( + @__MODULE__, + :(struct Foo x::Vector{<:Int} end), + :x, + ) # Fully specified UnionAll field type @test field_is_fully_specified( @@ -67,6 +97,11 @@ :(struct S{T1,T2} x::Dict{T1,T2} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1,T2} x::Vector{Dict{T1,T2}} end), + :x, + ) @test field_is_fully_specified( @__MODULE__, :(struct S{T0,T1,T2} x::Dict{T1,T2} end), @@ -77,22 +112,86 @@ :(struct S{T1} x::Dict{T1,Int} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Dict{T1,T1} end), + :x, + ) @test field_is_fully_specified( @__MODULE__, :(struct S{T2} x::Dict{Int,T2} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T2} x::Pair{Dict{Int,T2},T2} end), + :x, + ) + # where-clause on the field @test field_is_fully_specified( @__MODULE__, :(struct S{T1} x::Dict{T1, T2} where {T2<:Int} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Dict{T1, T2} where {T2<:Integer} end), + :x, + ) @test field_is_fully_specified( @__MODULE__, :(struct S{T1} x::Dict{T1,T2} where T2 end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Pair{Dict{K, Int64}, String} where K end), + :x, + ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Pair{Dict{K, Int64} where K, String} end), + :x, + ) + + # Type alias: + Foo{T} = Union{Vector{T}, Set{T}} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Foo{T} end), + :x, + ) + Bar{T} = Union{Vector{T}, Dict{T}} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Bar{T} end), + :x, + ) + Qux{T} = Union{Vector{T}, Dict{K, T} where K} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Qux{T} end), + :x, + ) + Baz{T} = Union{Vector{T}, Dict{Any, T}} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Baz{T} end), + :x, + ) + Xyz{T} = Union{Vector{T}, Dict{<:Any, T}} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Xyz{T} end), + :x, + ) + Opt{T} = Union{Vector{T}, Nothing} + @test field_is_fully_specified( + @__MODULE__, + :(struct S{T} x::Opt{T} end), + :x, + ) # False cases: @test false == field_is_fully_specified( @@ -110,6 +209,11 @@ :(struct S{T} x::Dict end), :x, ) + @test false == field_is_fully_specified( + @__MODULE__, + :(struct S{T1} x::Pair{Dict{Bool, Int64}} end), + :x, + ) # Not applicable (abstract type or no type at all): @test field_is_fully_specified( @@ -146,6 +250,11 @@ :(struct S x::Union{Vector{<:Any},Float64} end), :x, ) + @test field_is_fully_specified( + @__MODULE__, + :(struct S x::Union{Vector{T},Float64} where {T<:Integer} end), + :x, + ) end diff --git a/test/runtests.jl b/test/runtests.jl index af35024..be969d9 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,4 +1,4 @@ -using FullySpecifiedFieldTypesStaticTests +using StructFieldParamsTesting using ReTestItems -ReTestItems.runtests(FullySpecifiedFieldTypesStaticTests) +runtests(StructFieldParamsTesting; nworkers=1)