Check ambiguous methods in stdlib #28665

tkf · 2018-08-15T07:54:25Z

@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 · 2018-08-15T20:15:17Z

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 · 2018-08-16T04:45:00Z

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 · 2018-08-22T00:32:10Z

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 · 2018-08-23T01:04:20Z

test/ambiguous.jl

+# https://github.com/JuliaLang/julia/issues/27408
+@test length(detect_ambiguities(modules...; imported=true, recursive=true)) ≤ 2
+
+@test_broken detect_ambiguities(modules...; imported=true, recursive=true) == []


Perhaps isempty?

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?

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 · 2018-08-23T01:21:41Z

test/ambiguous.jl

+@test length(detect_ambiguities(modules...; imported=true, recursive=true)) ≤ 2
+
+# not using isempty so this prints more information when it fails
+@test_broken detect_ambiguities(modules...; imported=true, recursive=true) == []


This comment is response to #28665 (comment)

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

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.

tkf · 2018-11-24T03:32:47Z

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

vtjnash · 2018-11-24T04:22:32Z

test/ambiguous.jl

+    @eval import $lib
+end
+
+modules = @eval [$(Base._stdlibs...)]


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

Thanks. Good point. It makes the patch simpler.

chriscoey · 2019-08-17T00:27:26Z

bump

vtjnash · 2019-08-17T02:35:45Z

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

tkf · 2019-08-17T02:48:04Z

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

vtjnash · 2019-08-17T03:19:50Z

It needs to be rebased to get a new merge commit

tkf · 2019-08-17T03:29:36Z

Squashed & rebased.

tkf force-pushed the bound_ambiguities branch from b8792ee to d62db6a Compare August 15, 2018 08:32

tkf mentioned this pull request Aug 19, 2018

Resolve all method ambiguities in LinearAlgebra #28749

Merged

tkf force-pushed the bound_ambiguities branch from aab7f3d to e7ff79a Compare August 22, 2018 00:22

tkf mentioned this pull request Aug 22, 2018

CI test failure due to ambiguous method in ccall test #28804

Closed

Sacha0 reviewed Aug 23, 2018

View reviewed changes

Sacha0 approved these changes Aug 23, 2018

View reviewed changes

tkf force-pushed the bound_ambiguities branch from e7ff79a to c5d5be3 Compare August 23, 2018 01:19

tkf commented Aug 23, 2018

View reviewed changes

tkf force-pushed the bound_ambiguities branch from c5d5be3 to 5ed6ca9 Compare November 24, 2018 00:27

tkf changed the title ~~Bound number of ambiguous methods in stdlib~~ Check ambiguous methods in stdlib Nov 24, 2018

vtjnash reviewed Nov 24, 2018

View reviewed changes

vtjnash added the status:forget me not PRs that one wants to make sure aren't forgotten label Aug 17, 2019

tkf closed this Aug 17, 2019

tkf reopened this Aug 17, 2019

Bound number of ambiguous methods in stdlib

5a7185a

tkf force-pushed the bound_ambiguities branch from 089b424 to 5a7185a Compare August 17, 2019 03:28

andreasnoack merged commit 3eae681 into JuliaLang:master Aug 19, 2019

simeonschaub removed the status:forget me not PRs that one wants to make sure aren't forgotten label May 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check ambiguous methods in stdlib #28665

Check ambiguous methods in stdlib #28665

tkf commented Aug 15, 2018 •

edited

tkf commented Aug 15, 2018

tkf commented Aug 16, 2018

tkf commented Aug 22, 2018

Sacha0 Aug 23, 2018

tkf Aug 23, 2018

Sacha0 Aug 23, 2018 •

edited

Sacha0 left a comment

tkf Aug 23, 2018

Sacha0 Aug 23, 2018

tkf Aug 23, 2018

Sacha0 Aug 23, 2018

tkf commented Nov 24, 2018

vtjnash Nov 24, 2018

tkf Nov 24, 2018

chriscoey commented Aug 17, 2019

vtjnash commented Aug 17, 2019

tkf commented Aug 17, 2019

vtjnash commented Aug 17, 2019

tkf commented Aug 17, 2019

	# 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) == []

Check ambiguous methods in stdlib #28665

Check ambiguous methods in stdlib #28665

Conversation

tkf commented Aug 15, 2018 • edited

tkf commented Aug 15, 2018

tkf commented Aug 16, 2018

tkf commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Aug 23, 2018 • edited

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf commented Nov 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscoey commented Aug 17, 2019

vtjnash commented Aug 17, 2019

tkf commented Aug 17, 2019

vtjnash commented Aug 17, 2019

tkf commented Aug 17, 2019

tkf commented Aug 15, 2018 •

edited

Sacha0 Aug 23, 2018 •

edited