Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove overloading promote_type #426

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Conversation

lgoettgens
Copy link
Contributor

Resolves #425 in the way that is outlined in the docstrings

help?> Base.promote_type
  promote_type(type1, type2, ...)

  Promotion refers to converting values of mixed types to a single common type.
  promote_type represents the default promotion behavior in Julia when operators (usually
  mathematical) are given arguments of differing types. promote_type generally tries to
  return a type which can at least approximate most values of either input type without
  excessively widening. Some loss is tolerated; for example, promote_type(Int64, Float64)
  returns Float64 even though strictly, not all Int64 values can be represented exactly as
  Float64 values.

  See also: promote, promote_typejoin, promote_rule.

  [...]

  │ Don't overload this directly
  │
  │  To overload promotion for your own types you should overload promote_rule.
  │  promote_type calls promote_rule internally to determine the type. Overloading
  │  promote_type directly can cause ambiguity errors.

help?> Base.promote_rule
  promote_rule(type1, type2)

  Specifies what type should be used by promote when given values of types type1 and type2.
  This function should not be called directly, but should have definitions added to it for
  new types as appropriate.

@barche
Copy link
Collaborator

barche commented Apr 28, 2024

This change results in the error:

 basic_types: Error During Test at /home/runner/work/CxxWrap.jl/CxxWrap.jl/test/basic_types.jl:166
  Test threw exception
  Expression: typeof(a + a) == CxxWrap.CxxWrapCore.julia_int_type(CxxChar)
  promotion of types CxxChar and CxxChar failed to change any arguments

If I remember correctly that is also the reason I opted to override promote_type, but I am open to better solutions.

@fingolfin
Copy link
Contributor

That error pops up because there is no + method for CxxChar (or CxxLong, or...). Hence:

julia> a = CxxChar(3) ; typeof(a + a)
Int8

It seems to me the correct solution for that would be to provide suitable binary ops for those types, which then also could preserve the types. So this is the library code raising the error:

for op in (:+, :-, :*, :&, :|, :xor)
    @eval function $op(a::Integer, b::Integer)
        T = promote_typeof(a, b)
        aT, bT = a % T, b % T
        not_sametype((a, b), (aT, bT))
        return $op(aT, bT)
    end
end

you could adapt it to something like this:

for op in (:+, :-, :*, :&, :|, :xor)
    @eval function Base.$op(a::S, b::S) where {S<:CxxNumber}
        T = julia_int_type(S)
        aT, bT = a % T, b % T
        Base.not_sametype((a, b), (aT, bT))
        return S($op(aT, bT))
    end
end

lgoettgens and others added 2 commits June 11, 2024 12:05
Co-authored-by: Max Horn <max@quendi.de>
to keep this change non-breaking
@lgoettgens
Copy link
Contributor Author

Thanks @fingolfin for the help! Unfortunately that does not work directly as it coerces the results of arithmetic operations back to the Cxx type, but it is explicitly tested in

@test typeof(a+a) == CxxWrap.CxxWrapCore.julia_int_type(CxxChar)
that it returns julia types.
But just removing the last coercion seems to work.

@barche barche merged commit 2dbcf3c into JuliaInterop:main Jun 11, 2024
11 checks passed
@barche
Copy link
Collaborator

barche commented Jun 11, 2024

Thanks, well spotted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

promote_type is not supposed to be overloaded
3 participants