fix Aqua's reported piracies and method ambiguities

[Omar-Elrefaei]

Fix all of Aqua's reported piracies and method ambiguities. (#65)

Precisely 37 out of the 39 potential piracies are avoided by informing Aqua that types from QuantumInterface are in fact our own. The last two are fixed by adjusting the * and + functions that accept a Vararg, it had a problem for the case when N=0. This solution avoids both the unbound_type_param and the piracy, and seems to be a reasonable design to not allow zero arguments passed to the function (similar to Base operators themselves)

PR checklist

• The code is properly formatted and commented.
• Substantial new functionality is documented within the docs.
• [] All new functionality is tested.
• All of the automated tests on github pass.
• We recently started enforcing formatting checks. If formatting issues are reported in the new code you have written, please correct them. There will be plenty of old code that is flagged as we are slowly transitioning to enforced formatting. Please do not worry about or address older formatting issues -- keep your PR just focused on your planned contribution.

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.46%. Comparing base (c41ff6b) to head (9a814a5).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/QSymbolicsBase/basic_superops.jl	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
- Coverage   75.50%   75.46%   -0.04%     
==========================================
  Files          19       19              
  Lines         800      803       +3     
==========================================
+ Hits          604      606       +2     
- Misses        196      197       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[Krastanov]

Thank you for taking care of this! I have some minor questions about the types marked as own but on a quick skim the rest looks good.

[Krastanov]

Could you elaborate why these need to be marked as own? I am not sure whether this would be consistent with how independent packages downstream of QuantumInterface (e.g. QuantumOptics or QuantumCummulants) -- I want to make sure there are some precautions put in place for potential future issues.

[Omar-Elrefaei]

If I understand correctly, a type pyracy is when we expand a function definition with methods that take a certain type, but we neither defined the original function nor the type.
That was occurring all the time in files such as basic_ops_homogeneous.jl, basic_ops_inhomogeneous.jl, basic_superops.jl with methods that extend Base.(*) or Base.(+)

The last paragraph in the piracy section here gave me the impression that this was indeed happening intentionally. QuantumInterface defines the interface that is then used in other packages. So we tell Aqua that QuantumInterface and QuantumSymbolics are really cousin packages (since we avoided extending Base in QuantumInterface by design)

Aqua docs stating the following too gave me assurance that this is right.

this is useful for testing packages that deliberately commit some type piracies

Note: the functions really take Symbolic{T} not T directly, this seems to be worked around somewhere (same with Vector{T}) such that piracy detection is not tripped by these simple wrapper types.

[Krastanov]

Hi! Just a friendly bump on this. I see most of the piracies were masked by marking a bunch of types as owned. This will be a bit fragile. Do you have any insight into which piracies this addresses -- upstreaming them to QuantumInterface would be a safer long-term solution.

[Omar-Elrefaei]

Hi @Krastanov, I replied to your code review itself a while ago. Maybe GitHub doesn't show it too clearly #85 (comment)

Edit: wow it seems that I'm the one who has been duped by GitHub's UI, my apologies.

[Krastanov]

No worries, I have made the same mistake in the past.

[Omar-Elrefaei]

Motivation here was to avoid ambiguities with other packages,

But I'm not sure if it should be ::Number or ::{Union{Number, Symbolic{Number}}} to allow p*tr(q) just as tr(q)*p is allowed. The test suits for QuantumSymbolics and QuantumSavory pass right now, but I wasn't sure if that is a valid operation since it was allowed before my changes.

[Omar-Elrefaei]

If I understand correctly, a type pyracy is when we expand a function definition with methods that take a certain type, but we neither defined the original function nor the type.
That was occurring all the time in files such as basic_ops_homogeneous.jl, basic_ops_inhomogeneous.jl, basic_superops.jl with methods that extend Base.(*) or Base.(+)

The last paragraph in the piracy section here gave me the impression that this was indeed happening intentionally. QuantumInterface defines the interface that is then used in other packages. So we tell Aqua that QuantumInterface and QuantumSymbolics are really cousin packages (since we avoided extending Base in QuantumInterface by design)

Aqua docs stating the following too gave me assurance that this is right.

this is useful for testing packages that deliberately commit some type piracies

Note: the functions really take Symbolic{T} not T directly, this seems to be worked around somewhere (same with Vector{T}) such that piracy detection is not tripped by these simple wrapper types.

[Krastanov]

I see, that makes sense, thank you!

So an example of an offender would be

Base.:(*)(x::Symbolic{T}, y::Symbolic{S}) where {T<:QObj,S<:QObj}

I am wondering, can own_types take something like Symbolic{<:QI.AbstractBra}, in order to be more specific about what types are actually owned? After all, given the premise of this library, we can not really claim ownership of QI.AbstractBra, but Symbolic{<:QI.AbstractBra} is very much "owned".

[Omar-Elrefaei]

I'm won't be exactly sure why, but that doesn't seem to work.

At the simplest case Aqua.test_piracies(QuantumSymbolics, treat_as_own = [Symbolic{QI.AbstractOperator}]) still shows the following. (also tried with <:QI.AbstractOperator)

Possible type-piracy detected:
......
[35] tr(x::Symbolic{QuantumInterface.AbstractOperator}) @ QuantumSymbolics ~/.julia/dev/QuantumSymbolics/src/QSymbolicsBase/linalg.jl:305
[37] vec(x::Symbolic{QuantumInterface.AbstractOperator}) @ QuantumSymbolics ~/.julia/dev/QuantumSymbolics/src/QSymbolicsBase/linalg.jl:527

[Omar-Elrefaei]

After all, given the premise of this library, we can not really claim ownership of QI.AbstractBra

The way I'm thinking about it is that QuantumInterface is that one that owns AbstractBra in reality. If a user comes along and instantiates a Vector{<:AbstractBra} object, they still don't own it, and defining adjoint on it would still be a piracy.

What we are saying here is that QuantumSymbolics gets a special treatment because it is really an extension of QuantumInterface by the same developers. QuantumInterface itself says This package is not meant for public use! in the readme. I'm assuming non of your other quantum packages will ever be interested in defining Base.:(*) too.

[Krastanov]

You are right, it is just that there are multiple groups coordinating on this work, and having as much of the coordination automated would help a lot.

In particular, if we just treat_as_own the bare AbstractKet, we will miss actual issues like f(::AbstractKet) which should be in QuantumInterface not in QuantumSymbolics, making Aqua less helpful than it could be -- this is my main worry.

I posted about this on slack. Let's wait a bit to see if anyone has suggestions there.

[Krastanov]

Actually, how about we keep your implementation as is, but also add the following extra test?

all([m.sig.types[2:end] ⊆ own_types for m in Aqua.Piracy.hunt(QuantumSymbolics)])

The subset check might be too harsh. An intersection check might be enough.

[Omar-Elrefaei]

we will miss actual issues like f(::AbstractKet)

Ok, that is the use case I didn't think of on my own

but also add the following extra test?

Yeah, doesn't seem like a bad idea. I went through few iterations to get to something that doesn't show false-positives.

# get it to work without errors
all([Base.unwrap_unionall(m.sig).types[2:end] ⊆ own_types for m in Aqua.Piracy.hunt(QuantumSymbolics)])

get_method_args(m::Method) = Base.unwrap_unionall(m.sig).types[2:end] # for readability here

# Transform to filter to see the cases that are not passing
filter(m -> !(get_method_args(m) ⊆ own_types), Aqua.Piracy.hunt(QuantumSymbolics)) # 18 false-positives

# Use intersection test. Subset is too strict because it keeps out methods which take additional arguments
#not related to QuantumSymbolics.  Example ptrace(x::Symbolic{QuantumInterface.AbstractOperator}, s)
filter(m -> isdisjoint(get_method_args(m), own_types), Aqua.Piracy.hunt(QuantumSymbolics)) #10 false-positives

# Some arguments need a little normalization. Such as maketerm(::Type{<:Symbolic{<:QObj}},...)
function normalize(arg)
    if (arg isa UnionAll) && (arg.body <: Type) arg = arg.body.parameters[1] end
    if (arg isa Core.TypeofVararg) arg = arg.T end
    if (arg isa TypeVar) arg = arg.ub end
    return arg
end

filter(m -> isdisjoint(normalize.(get_method_args(m)), own_types), Aqua.Piracy.hunt(QuantumSymbolics)) #8 false-positives

# What we really need is to check if an arg is a subtype of Symbolic{<:QObj}, to be able to match most edge cases
filter(m -> !any(normalize.(get_method_args(m)) .<: Symbolic{<:QObj}), Aqua.Piracy.hunt(QuantumSymbolics)) # 0 false-positives

Here is a proof of it catching an introduced piracy

module modA
       import QuantumInterface: AbstractBra, AbstractKet
       import Base.println
       import SymbolicUtils.Symbolic
       Base.println(x::T) where T<:Union{AbstractBra, AbstractKet} = 1
       Base.println(x::Symbolic{T}) where T<:Union{AbstractBra, AbstractKet} = 2
end

filter(m -> !any(normalize.(get_method_args(m)) .<: Union{Symbolic{<:QObj}}), Aqua.Piracy.hunt(modA))
#[1] println(x::T) where T<:Union{AbstractBra, AbstractKet} @ Main.modA REPL[53]:5

[Krastanov]

This is wonderfully done, thank you for working it out!

[Omar-Elrefaei]

I'm good with this. Might have gotten carried away.

[Krastanov]

Thank you, this is very much appreciated.

Could you message me at [email protected] with your name, github username, and bounty number so I can forward you the bounty payment details?