Check ambiguous methods in stdlib

[tkf]

closes #27408

---

@Sacha0 I added the tests I suggested in #27408.

The nice thing I noticed is that the number of ambiguous methods is now dropped to 13 (from 25)!

[tkf]

I'm puzzled that my change invokes test failures in very different places:

Error in testset ambiguous:
Test Failed at /tmp/julia/share/julia/test/ambiguous.jl:238
  Expression: length(detect_ambiguities(Ambig7)) == 1
   Evaluated: 0 == 1
Error in testset ambiguous:
Test Failed at /tmp/julia/share/julia/test/ambiguous.jl:280
  Expression: length(detect_ambiguities(Ambig9, ambiguous_bottom=true)) == 1
   Evaluated: 0 == 1
ERROR: LoadError: Test run finished with errors
in expression starting at /tmp/julia/share/julia/test/runtests.jl:61

--- https://travis-ci.org/JuliaLang/julia/jobs/416266235#L2059

Those happen at the code way after the code I changed. Also, I don't see how those tests can interact with the one I added:

julia/test/ambiguous.jl

Lines 233 to 238 in d62db6a

	module Ambig7
	struct T end
	(::T)(x::Int8, y) = 1
	(::T)(x, y::Int8) = 2
	end
	@test length(detect_ambiguities(Ambig7)) == 1

julia/test/ambiguous.jl

Lines 272 to 281 in d62db6a

	module Ambig9
	f(x::Complex{<:Integer}) = 1
	f(x::Complex{<:Rational}) = 2
	end
	@test !Base.isambiguous(methods(Ambig9.f)..., ambiguous_bottom=false)
	@test Base.isambiguous(methods(Ambig9.f)..., ambiguous_bottom=true)
	@test !Base.isambiguous(methods(Ambig9.f)...)
	@test length(detect_ambiguities(Ambig9, ambiguous_bottom=false)) == 0
	@test length(detect_ambiguities(Ambig9, ambiguous_bottom=true)) == 1
	@test length(detect_ambiguities(Ambig9)) == 0

I'll try to debug it but please let me know if I'm making some obvious mistakes.

[tkf]

OK. So tests are all fixed now and CIs are all green. @Sacha0 Can you merge this?

FYI: The puzzling behavior was due to this silly mistake:
1721f74

[tkf]

Now that #28749 is merged (thanks @andreasnoack!), there are only two ambiguous methods left (occurring in SparseArrays).

julia> detect_ambiguities(Base64, CRC32c, Dates, DelimitedFiles, Distributed, FileWatching,
                  Future, InteractiveUtils, Libdl, LibGit2, LinearAlgebra, Logging,
                  Markdown, Mmap, Pkg, Printf, Profile, Random, REPL,
                  Serialization, SHA, SharedArrays, Sockets, SparseArrays, Statistics,
                  SuiteSparse, Test, Unicode, UUIDs; imported=true, recursive=true)
Skipping Distributed.cluster_manager
Skipping Random.DSFMT.win32_SystemFunction036!
2-element Array{Tuple{Method,Method},1}:
 (capturescalars(f, mixedargs::Tuple{Ref{Type{T}},Vararg{Any,N} where N}) where T in SparseArrays.HigherOrderFns at /home/takafumi/repos/watch/julia/usr/share/julia/stdlib/v1.1/SparseArrays/src/higherorderfns.jl:1025, capturescalars(f, mixedargs::Tuple{Union{AbstractArray{0,N} where N, Ref},Ref{Type{T}},Vararg{Any,N} where N}) where T in SparseArrays.HigherOrderFns at /home/takafumi/repos/watch/julia/usr/share/julia/stdlib/v1.1/SparseArrays/src/higherorderfns.jl:1029)
 (_copy(f, args::SparseVector...) in SparseArrays.HigherOrderFns at /home/takafumi/repos/watch/julia/usr/share/julia/stdlib/v1.1/SparseArrays/src/higherorderfns.jl:968, _copy(f, args::SparseMatrixCSC...) in SparseArrays.HigherOrderFns at /home/takafumi/repos/watch/julia/usr/share/julia/stdlib/v1.1/SparseArrays/src/higherorderfns.jl:969)

I changed the bound and then rebase onto master. I may have a go at SparseArrays sometime later but I think we still need this global test to avoid further ambiguities. If it's solved in SparseArrays we just need to swap @test_broken with @test.

[Sacha0]

Perhaps isempty?

[tkf]

I followed the example for Base and Core as I thought it makes sense:

julia/test/ambiguous.jl

Lines 154 to 156 in f31e28a

	# Test that Core and Base are free of ambiguities
	# not using isempty so this prints more information when it fails
	@test detect_ambiguities(Core, Base; imported=true, recursive=true, ambiguous_bottom=false) == []

Shall I add a comment to clarify it?

[Sacha0]

No need for a comment; it's a matter of style really :). I'm a fan of using evocatively named predicate functions where they exist. Best! (Edit: Oh gosh, apologies, I completely missed the relevant comment! Kindly ignore this comment :).)

[Sacha0]

LGTM per a brief review! Thanks @tkf! :)

[tkf]

This comment is response to #28665 (comment)

[Sacha0]

Oh gosh, apologies, I completely missed the relevant comment in the code snippet you linked above! Absolutely, good call.

[tkf]

The relevant code for Base and Core is something like 20 lines above so I think it makes sense to repeat the comment. Someone replacing @test_broken with @test might just swap it with isempty if there is no comment.

[Sacha0]

Agreed! :)

[tkf]

Now that #30120 is merged, there is no method ambiguities in stdlib. Isn't it the best timing to merge this?

[vtjnash]

I think we should call the Base loaded_modules function here, instead of a list of symbols.

[tkf]

Thanks. Good point. It makes the patch simpler.

[chriscoey]

bump

[vtjnash]

Whoops, yes we should have. It needs to be rebased though now, so we can check if it is still working.

[tkf]

There is no conflicts so I guess re-running CI is sufficient?

[vtjnash]

It needs to be rebased to get a new merge commit

[tkf]

Squashed & rebased.