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

applicable and hasmethod is returning wrong information for the function round in Julia master #51258

Closed
ronisbr opened this issue Sep 10, 2023 · 13 comments · May be fixed by #51260
Closed

applicable and hasmethod is returning wrong information for the function round in Julia master #51258

ronisbr opened this issue Sep 10, 2023 · 13 comments · May be fixed by #51260
Labels
docs This change adds or pertains to documentation

Comments

@ronisbr
Copy link
Member

ronisbr commented Sep 10, 2023

Hi!

I am seeing build errors in PrettyTables.jl using Julia master. After some analysis, it turns out that the functions applicable and hasmethod is indicating that the function round can be used with arguments of type String:

Julia nightly

julia> applicable(round, "test")
true

julia> hasmethod(round, (String,))
true

julia> round("test")
ERROR: MethodError: no method matching round(::String, ::RoundingMode{:Nearest})

Closest candidates are:
  round(::Type{T}, ::Any, ::RoundingMode) where T>:Missing
   @ Base missing.jl:147
  round(::Type{T}, ::Any, ::RoundingMode) where T
   @ Base rounding.jl:470
  round(::BigFloat, ::RoundingMode{:Nearest})
   @ Base mpfr.jl:1032
  ...

Stacktrace:
 [1] round(x::String; kws::@Kwargs{})
   @ Base ./rounding.jl:463
 [2] top-level scope
   @ REPL[3]:1

Julia 1.9

julia> applicable(round, "Teste")
false

julia> hasmethod(round, (String,))
false
@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 10, 2023

Likely an unintended consequence of #50812, @LilithHafner

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

Indeed! If I pass all the arguments, it provides the correct answer:

julia> applicable(round, "test", RoundNearest)
false

julia> applicable(round, 1.0, RoundNearest)
true

@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 10, 2023

The issue is that there now truly is a method round(x) that's generic enough to take a String, since it takes Any. The resulting call will error for other reasons, but the refactor resulted in making this "entry point" applicable.

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

Yes, this is the line you mentioned:

round(x; kws...) = round(x, RoundNearest; kws...)

Of course I can easily change this in PrettyTables.jl but wouldn't it be a really breaking change?

@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 10, 2023

I don't know. What exactly are you relying on here? Adding new methods to functions in Base is, as far as I recall, not a breaking change. That'd be incredibly limiting.

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

When we are formatting cells in a Table, I do not know what is the cell type. We have a feature called formatters that pre-process the cell before converting it to a string. Hence, the user can, for example, select a better numerical representation. Sometimes, the user wants to apply a formatter that rounds all the cells to a certain number of digits. Thus, before calling round, we need to check if round is applicable to the provided cell type to avoid errors. The code is:

                    if applicable(round, v)
                        return round(v, digits = digits[k])
                    else
                        return v
                    end

But notice that it is only inside a pre-defined formatter in PrettyTables.jl. Maybe users of the package already defined formatters using the same "recipe" in other cases, which will badly break with 1.11.

Adding new methods to functions in Base is, as far as I recall, not a breaking change. That'd be incredibly limiting.

I agree that adding methods to Base should not be considered breaking, but this is changing the behavior of a already defined method.

@KristofferC
Copy link
Member

KristofferC commented Sep 10, 2023

Using applicable for stuff like this doesn't really make sense (as is obvious by the problem encountered in this issue).

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

Using applicable for stuff like this doesn't really make sense (as is obvious by the problem encountered in this issue).

Then I think the documentation must state this clearly:

help?> applicable
search: applicable

  applicable(f, args...) -> Bool

  Determine whether the given generic function has a method applicable to the given arguments.

  See also hasmethod.

help?> hasmethod
search: hasmethod

  hasmethod(f, t::Type{<:Tuple}[, kwnames]; world=get_world_counter()) -> Bool

  Determine whether the given generic function has a method matching the given Tuple of argument types with the upper bound of world age given by world.

  If a tuple of keyword argument names kwnames is provided, this also checks whether the method of f matching t has the given keyword argument names. If the
  matching method accepts a variable number of keyword arguments, e.g. with kwargs..., any names given in kwnames are considered valid. Otherwise the
  provided names must be a subset of the method's keyword arguments.

  See also applicable.

For anyone not familiar with the internals, it seems very clear that if applicable returns true, the method will work, which is not the case.

ronisbr added a commit to ronisbr/julia that referenced this issue Sep 10, 2023
This commit adds a warning about the functions `applicable` and
`hasmethod`. A user reading the current documentation can think that if
those functions return `true`, we can call the method without errors.
However, it is not always the case and should be clearly stated.

Closes JuliaLang#51258
ronisbr added a commit to ronisbr/PrettyTables.jl that referenced this issue Sep 10, 2023
@brenhinkeller brenhinkeller added the docs This change adds or pertains to documentation label Sep 14, 2023
@bvdmitri
Copy link
Contributor

bvdmitri commented Sep 17, 2023

I think the real problem here is the implicit assumption that an applicable method executes without an error. The documentation is not lying, there is indeed an applicable method, but it simply results in an error.

To make this confusion a bit more explicit consider this function:

throw_if_int(::Int) = error("not applicable")
throw_if_int(::Any) = 0

Both applicable and hasmethod will correctly report true for Int, even though you get exception if you actually call this method. Something similar happens with the round for the String now.

EDIT: Ah I can see now that you've discussed in in the PR.

@aplavin
Copy link
Contributor

aplavin commented Sep 17, 2023

Sometimes, the user wants to apply a formatter that rounds all the cells to a certain number of digits

Sounds like x isa Number is a more straightforward and reliable check then.
There are lots of functions with methods completely/almost without type constraints in Julia. It's not reasonable to assume that if f(>:T) exists then calling f(t) wouldn't throw an error.

@ronisbr
Copy link
Member Author

ronisbr commented Sep 17, 2023

The problem is that people might overload round for custom types (I myself did this in some analysis). In this case, applicable should be the only way without using try and catch.

@LilithHafner
Copy link
Member

Probably not the best idea, but you could use this:

julia> function has_round(x) 
           isempty(Base.return_types(round, Tuple{typeof(x)})) && return false
           try
               round(x)
               true
           catch
               false
           end
       end
has_round (generic function with 1 method)

julia> using Random, BenchmarkTools, PrettyTables

julia> @btime has_round(x) setup=(x=rand())
  402.915 ns (8 allocations: 656 bytes)
true

julia> @btime has_round(x) setup=(x=randstring())
  421.271 ns (5 allocations: 464 bytes)
false

julia> @btime has_round(x) setup=(x=rand([Base.Forward, Base.DEFAULT_STABLE, Base, open]))
  441.919 ns (5 allocations: 464 bytes)
false

julia> @btime applicable(round, x, RoundNearest) setup=(x=randstring())
  482.262 ns (0 allocations: 0 bytes)
false

julia> io = IOBuffer();

julia> table = randn(5,5);

julia> @btime pretty_table(io, table)
  41.084 μs (1024 allocations: 58.46 KiB)

But be aware that Base.return_types is internal.

@KristofferC
Copy link
Member

applicable and hasmethod is doing the correct thing here so I think this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants