Compat interfers with dictionary type inference

[cstjean]

On 0.4, the Dict key-type is wrong after using Compat

julia> [Symbol(k) => v for (k,v) in Dict{Any, Any}(:x=>3)]
Dict{Symbol,Any} with 1 entry:
  :x => 3

julia> using Compat

julia> [Symbol(k) => v for (k,v) in Dict{Any, Any}(:x=>3)]
Dict{Any,Any} with 1 entry:
  :x => 3

[yuyichao]

Dup of JuliaLang/julia#7258 Not really fixable.

[nalimilan]

@yuyichao But why would using Compat change this? Sounds like an unexpected (and undocumented) outcome.

[yuyichao]

Any method definition can change this. Type inference was just being nice about guessing Symbol(::Any).

[cstjean]

The problem comes from this line. On Julia 0.4.5, Symbol is already defined, so if we lowered the version number it would solve the issue.

[yuyichao]

No. The whole point of that line is to add method to Symbol.

[nalimilan]

Maybe we could add methods with more specific signatures? On 0.4, only the default Symbol(::String) constructor is defined: looks like it's possible to avoid messing with it.

[nalimilan]

More specifically, of the defined methods on 0.5, 0.4 already has the first. We only need to add the others:

Symbol(s::String) at coreimg.jl:52
Symbol(a::Array{UInt8,1}) at coreimg.jl:53
Symbol(s::AbstractString) at strings/basic.jl:77
Symbol(x...) at sysimg.jl:74

[yuyichao]

No. As I said, the whole point is to add methods to Symbol (so that it matches the signature of Symbol on 0.5). For this particular one, you can add a type assertion but the issue of adding method inferring with type inf is not fixable in Compat.

[yuyichao]

And therefore we added Symbol(x...) at sysimg.jl:74

[cstjean]

Replacing the compat definition with

Base.Symbol(a, b, args...) = symbol(a, b, args...)

fixes the inference bug. It's ugly and might have other consequences, so I don't know if it's appropriate...

[yuyichao]

fixes the inference bug

It's not a bug.

might have other consequences

It does

so I don't know if it's appropriate...

We shouldn't do that.

And as I said,

For this particular one, you can add a type assertion

[cstjean]

Alright, alright. Thank you for the explanation.

[nalimilan]

It's clearly a bug, as in "not intended, nor documented, and annoying". It may or may not be fixable/workaround-able, but that's a separate question. Why do you think we shouldn't do that?

[yuyichao]

The bug is JuliaLang/julia#7258 and yes we should fix it.

[nalimilan]

We won't fix it in 0.4 though, so the bug is in Compat.

[yuyichao]

As a fact, adding any method makes type inference worse when the input is not inferenced as leaf type. If this counts as a bug in Compat, the only fix is to not add any methods in Compat at all.

[nalimilan]

What's the problem with the workaround suggested by @cstjean?

[yuyichao]

It's the wrong workaround (try Symbol("aaa".data)). And

For this particular one, you can add a type assertion

[nalimilan]

Let's add a type assertion then?

[yuyichao]

That's what I'm saying. Note that you might get complaint if someone extends symbol, which is basically as undefined as (edit:) slightly less undefined than relying on the type inference result of Symbol(::Any)

[nalimilan]

Extending symbol to return another type than Symbol sounds like a bad idea anyway. @cstjean Could you experiment with that solution and make a pull request?