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

improve method name printing in edge cases #14884

Merged
merged 3 commits into from
Feb 2, 2016
Merged

improve method name printing in edge cases #14884

merged 3 commits into from
Feb 2, 2016

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 1, 2016

this fixes method printing for errors and a few edge cases.
fixed printing includes:

julia> Base.Symbol() = 1
julia> (::Symbol)() = 2
julia> Base.Int() = 3
julia> (::Int)() = 4

julia> which(Symbol, Tuple{})
Symbol() at none:1

julia> which(:a, Tuple{})
(::Symbol)() at none:1

julia> which(Int, Tuple{})
Int64() at none:1

julia> which(3, Tuple{})
(::Int64)() at none:1

julia> function f(x)
                g() = x
              end
f (generic function with 2 methods)

julia> which(f(1), Tuple{})
g() at none:2

these fixes are repeated for stacktrace, MethodError, and writemime

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

Corner case:

julia> type A
       end

julia> type B <: Function
       end

julia> (::A)(x) = throw(x)

julia> (::B)(x) = throw(x)

julia> A()(1)
ERROR: 1
 in (::A)(::Int64) at ./none:1
 in eval at ./boot.jl:267

julia> B()(1)
ERROR: 1
 in B(::Int64) at ./none:1
 in eval at ./boot.jl:267

Not sure how to distinguish between a user defined (singleton) Function type and a plain old generic function...

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

This appears on both master and this branch:

julia> type B <: Function
       end

julia> B()
B (generic function with 0 methods)

@vtjnash
Copy link
Member Author

vtjnash commented Feb 1, 2016

some other possible conditionals that might help:
ft.name.name != ft.name.mt.name
ft == typeof(getfield(ft.name.module, name)) == getfield(ft.name.module, ft.name.name)

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

ft.name.name !== ft.name.mt.name (with a comment) sounds good. Would also be nice to add some tests since these feels fairly fragile....

@vtjnash
Copy link
Member Author

vtjnash commented Feb 1, 2016

hm, and here i was going to go with ft == typeof(getfield(ft.name.module, name))

either should fix your example, but may make closures and the kwsorter less optimal:

julia> (typeof(sum).name.mt.kwsorter)
#sum (generic function with 1 method)

julia> methods(typeof(sum).name.mt.kwsorter)
# 1 method for generic function "#sum":
(::Base.#kw##sum)(::Array{T<:Any,N<:Any}, ::Base.#sum)

however, the kwsorter doesn't have line-numbers, so it is hidden from backtraces anyways

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

@vtjnash vtjnash force-pushed the jn/fn_error_msgs branch 2 times, most recently from afb1769 to 5ecb288 Compare February 1, 2016 07:05
@vtjnash
Copy link
Member Author

vtjnash commented Feb 1, 2016

yes, although i forgot in 0a9e6f9 why i initially hadn't done that. should be better now.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

Showing a custom Function type as generic function is still a little bit strange, especially if it is not a singleton.

julia> type B <: Function
       end

julia> B()
(::B) (generic function with 0 methods)

julia> type C <: Function
           x
       end

julia> C(1)
(::C) (generic function with 0 methods)

@vtjnash
Copy link
Member Author

vtjnash commented Feb 1, 2016

The way I see it, <: Function is explicitly opting in to this representation. The representation issue is that we don't know the name for it, whereas with normal functions, we have (sin::#sin).

@yuyichao
Copy link
Contributor

yuyichao commented Feb 1, 2016

Fair enough given that we don't print the captured variables for closures either.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 2, 2016

Test failure is #14592

@yuyichao
Copy link
Contributor

yuyichao commented Feb 2, 2016

Looks like some test infrastructure is also confused though.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 2, 2016

From the code it looks like we neither recompile the function nor trying to get it from the sysimg?

Seems that OSX CI passes since we are not running the reflection test and windows tests passes since we don't have a sysimg library....

@vtjnash
Copy link
Member Author

vtjnash commented Feb 2, 2016

rebased to try again on CI -- and all looks good (other than the travis worker that failed to start)


method_defs_lineno = @__LINE__
Base.Symbol() = error(1)
(::Symbol)() = error(2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be touching base methods on base types in the tests like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it comes up, we can add a dummy argument (perhaps EightBitType). but for now, i specifically want to test Symbol since it has a special and unique representation.

@tkelman
Copy link
Contributor

tkelman commented Feb 2, 2016

Travis issue was a problem during the initial apt-get install of the ppa gcc version that we need due to recent llvm.

I guess Symbol() is already a MethodError so maybe not too bad to mess with it in the tests, for now.

vtjnash added a commit that referenced this pull request Feb 2, 2016
improve method name printing in edge cases
@vtjnash vtjnash merged commit d7ee828 into master Feb 2, 2016
@vtjnash vtjnash deleted the jn/fn_error_msgs branch February 2, 2016 16:29
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.

3 participants