methods with unbound static parameters

[nsajko]

julia> using Test: Test

julia> using FastDifferentiation: FastDifferentiation as FastD

julia> Test.detect_unbound_args(FastD, recursive=true)
[1] derivative(A::FastDifferentiation.Node, variables::T...) where T<:Node @ FastDifferentiation ~/tmp/jl/FastDifferentiation.jl/src/Jacobian.jl:387
[2] make_variables(name::Symbol, array_size::T...) where T @ FastDifferentiation ~/tmp/jl/FastDifferentiation.jl/src/ExpressionGraph.jl:634
[3] (::FastDifferentiation.var"#set_dom_bits!#40")(subgraph::Union{Nothing, Tuple{T, T}}, all_subgraphs::Dict, var_or_root_index::Integer, bit_dimension::Integer) where T<:Integer @ FastDifferentiation ~/tmp/jl/FastDifferentiation.jl/src/Factoring.jl:153
[4] derivative(A::AbstractArray{<:FastDifferentiation.Node}, variables::T...) where T<:Node @ FastDifferentiation ~/tmp/jl/FastDifferentiation.jl/src/Jacobian.jl:374

[nsajko]

NB: in all four methods the relevant static parameter is only unbound for some invocations. E.g., bound for nonempty Vararg but unbound for empty Vararg.

[brianguenter]

I've never used the detect_unbound_args function before, trying to understand how important it is to fix this.

What does it mean for a parameter to be unbound? When Vararg is empty the parameter has the value (), an empty Tuple.

In general if you have a Vararg parameter the expectation has to be that it might be empty. Why would this be an error that needs to be corrected? If it is then does this mean that you should never have Vararg parameters in this form? Do they all have to be more like this param::Tuple{T,Vararg{T}} where{T}, which would force at least one argument?

Is this a false positive by detect_unbound_args?

[nsajko]

how important it is to fix this.

Not sure without taking a look at the code, but it'd be a good practice to fix those.

What does it mean for a parameter to be unbound?

It means it has no value.

When Vararg is empty the parameter has the value (), an empty Tuple.

No, the parameter T is the element type. The issue is that the element type can't be deduced for an empty Vararg.

In general if you have a Vararg parameter the expectation has to be that it might be empty.

Yes.

Why would this be an error that needs to be corrected?

A static parameter being unbound during the invocation of a method can slow down the call, if I'm not mistaken. Trying to use an unbound static parameter in the method body will obviously result in an error. If you're sure that the method only gets called in such a manner that the static parameter is bound, you could ignore this. However then you stand to miss actual issues in the future, so it'd be best to have no potential unbound parameters. Also see the Aqua.jl package, often used in a package's test suite: https://juliahub.com/ui/Packages/General/Aqua

If it is then does this mean that you should never have Vararg parameters in this form?

This form is just fine, as long as the method parameter is otherwise sufficiently constrained.

Do they all have to be more like this param::Tuple{T,Vararg{T}} where{T}, which would force at least one argument?

Yeah, this would certainly be fine, although adding an extra argument would be less of a hassle, like this: first::T, rest::T.... Then T would be bound even if rest is empty.

Is this a false positive by detect_unbound_args?

I'd say it's a true positive, at least looking at the method in isolation.

[nsajko]

BTW, taking a look at one of the flagged methods:

FastDifferentiation.jl/src/ExpressionGraph.jl

Lines 634 to 641 in 91857e7

	function make_variables(name::Symbol, array_size::T...) where {T}
	result = Array{Node,length(array_size)}(undef, array_size...)
	for i in CartesianIndices((UnitRange.(1, array_size)))
	result[i] = Node(Symbol(name, join(i.I, "_")))
	end
	return result
	end

The type parameter isn't used at all in the method body, so it's possible just not using a type parameter would be best.