JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.65k stars 5.48k forks source link

atsign-code* should dive into calls with filled optional/keyword args? #17127

Open timholy opened 8 years ago

timholy commented 8 years ago

I was coaching one of the GSoC students through analysis of a type-instability, and more acutely noticed the obstacle that default and keyword arguments impose on this kind of debugging. Here's an example which I've deliberately designed to be misleading:

function foo(A, label="Hi")
    println("label")
    sz = size(A)[1:end-1]
    B = rand(sz)
    sum(B)::eltype(A)
end

julia> @code_warntype foo(A)
Variables:
  #self#::#foo
  A::Array{Float64,3}

Body:
  begin  # /tmp/unstable.jl, line 2:
      return $(Expr(:invoke, foo(::Array{Float64,3}, ::String), :(#self#), :(A), "Hi"))
  end::Float64

Especially if you're not used to reading typed output, this gives the impression of being fine: there's no red anywhere. Of course, all we're really looking at is the function that fills in the default argument. If we supply the optional argument, then we obtain

julia> @code_warntype foo(A, "hello")
Variables:
  #self#::#foo
  A::Array{Float64,3}
  label::String
  sz::Tuple{Vararg{Int64,N}}
  B::Any

Body:
  begin  # /tmp/unstable.jl, line 2:
      # meta: location coreio.jl println # coreio.jl, line 5:
      SSAValue(1) = (Core.typeassert)(Base.STDOUT,Base.IO)::IO
      # meta: pop location
      (Base.print)(SSAValue(1),"label",'\n')::Void # line 3:
      # meta: location array.jl size # array.jl, line 20:
      # meta: location array.jl _size # array.jl, line 22:
      SSAValue(7) = (Base.arraysize)(A::Array{Float64,3},(Base.box)(Int64,(Base.add_int)(0,1)))::Int64
      # meta: location array.jl _size # array.jl, line 22:
      SSAValue(5) = SSAValue(7)
      SSAValue(6) = (Base.arraysize)(A::Array{Float64,3},(Base.box)(Int64,(Base.add_int)(1,1)))::Int64
      # meta: location array.jl _size # array.jl, line 22:
      SSAValue(4) = (Core.tuple)(SSAValue(5),SSAValue(6),(Base.arraysize)(A::Array{Float64,3},(Base.box)(Int64,(Base.add_int)(2,1)))::Int64)::Tuple{Int64,Int64,Int64}
      # meta: pop location
      # meta: pop location
      # meta: pop location
      # meta: pop location
      SSAValue(0) = SSAValue(4)
      SSAValue(8) = (Base.nfields)(SSAValue(0))::Int64
      SSAValue(9) = (Base.box)(Int64,(Base.sub_int)(SSAValue(8),1))
      sz::Tuple{Vararg{Int64,N}} = $(Expr(:invoke, getindex(::Tuple{Int64,Int64,Int64}, ::UnitRange{Int64}), :(Main.getindex), SSAValue(0), :($(Expr(:new, UnitRange{Int64}, 1, :((Base.select_value)((Base.sle_int)(1,SSAValue(9))::Bool,SSAValue(9),(Base.box)(Int64,(Base.sub_int)(1,1)))::Int64)))))) # line 4:
      B::Any = (Base.Random.rand!)(Base.Random.GLOBAL_RNG,((Core.apply_type)(Base.Random.Array,Base.Random.Float64)::Type{Array{Float64,N}})(sz::Tuple{Vararg{Int64,N}})::Array{T,N})::Any # line 5:
      return (Core.typeassert)((Main.sum)(B::Any)::Any,Float64)::Float64
  end::Float64

and we see lots of red (the size(A)[1:end-1] introduces a type-instability).

It seems like it might be useful to provide (via an option?) the ability to "dive down" into the call that gets produced once all optional arguments have been filled in. I'm not exactly sure what that should look like, or what the default behavior should be.

yuyichao commented 8 years ago

Not sure if we still want this but at least this should not have been closed by #17346 (@github not fix #... is quite different from fix #...)...

simonster commented 8 years ago

Oops, I didn't notice that. Yes, I think we still want this.