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

RFC: Don't suggest other type constructors in closest method suggestions in REPL #16155

Merged
merged 1 commit into from
May 10, 2016

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented May 1, 2016

#16118 (comment) shows how for type constructors, the current code that suggests closest methods upon MethodError walks through ALL constructors and counts how many arguments match.

This means that type constructors that for example take a bunch of Any will always have all it's argument match and always be suggested as a top closest candidate:

julia> type Foo; a; end

julia> Foo(1,2)
ERROR: MethodError: no method matching Foo(::Int64, ::Int64)
Closest candidates are:
  (::Type{BoundsError})(::ANY, ::ANY)
  (::Type{TypeError})(::Any, ::Any, ::Any, ::Any)
  (::Type{TypeConstructor})(::ANY, ::ANY)
  ...
 in eval(::Module, ::Any) at ./boot.jl:243

This PR would simply not print a closest match for anything except for the type constructor, like this:

julia> type Foo2; a; b; end

julia> Foo(1,2) # Foo2 or any other constructor is not suggested
ERROR: MethodError: no method matching Foo(::Int64, ::Int64)
Closest candidates are:
  Foo(::Any)
  Foo{T}(::Any)
 in eval(::Module, ::Any) at ./boot.jl:236

This is consistent with how functions are being suggested. We don't list all other functions that happened to have arguments match so why should we do it for type constructors?

julia> foo(a::Int) = a
foo (generic function with 1 method)

julia> foo2(a::Float64) = a
foo2 (generic function with 1 method)

julia> foo(1.0) # No closest method matches
ERROR: MethodError: no method matching foo(::Float64)
 in eval(::Module, ::Any) at ./boot.jl:237

It also matches how I personally always use the suggestions which is that I mess up with the type for one of the fields in a type with a lot of fields and I want to know which field I messed up on.

Further comments and ideas are welcome.

@tkelman
Copy link
Contributor

tkelman commented May 3, 2016

are there tests for the printing of these errors?

@KristofferC
Copy link
Member Author

KristofferC commented May 3, 2016

Since this has regressed since 0.4 and my changes passed without any changed tests I would say no :P

@KristofferC
Copy link
Member Author

KristofferC commented May 3, 2016

I will add tests if this is likely to get merged.

It would be interesting if anyone has an example where this PR provides a worse suggestion than the current one on master.

Comparing to 0.4 this would not have the call and convert methods listed below. I don't see much value in them though.

julia> Foo(1,2) # On v0.4
ERROR: MethodError: `convert` has no method matching convert(::Type{Foo}, ::Int64, ::Int64)
This may have arisen from a call to the constructor Foo(...),
since type constructors fall back to convert methods.
Closest candidates are:
  Foo(::Any)
  call{T}(::Type{T}, ::Any)
  convert{T}(::Type{T}, ::T)
 in call at essentials.jl:57

@Tetralux
Copy link
Contributor

Tetralux commented May 3, 2016

I would think that it would be useful to continue showing applicable overloads for convert after the constructors.

@KristofferC
Copy link
Member Author

Could you give an example?

@Tetralux
Copy link
Contributor

Tetralux commented May 3, 2016

@KristofferC Scratch that. For some reason I thought this was more general than it is.
I agree; there's no reason to display overloads for convert after tab-completion here.

Edit: I really shouldn't comment while tired...

@KristofferC
Copy link
Member Author

This is not about tab completion but about what suggestions to give after a MethodError.

@Tetralux
Copy link
Contributor

Tetralux commented May 3, 2016

@KristofferC Sorry, yes; that's what I meant.

@Tetralux
Copy link
Contributor

Tetralux commented May 3, 2016

@KristofferC Hmmm... After giving it some further thought, I actually did come up with an example:

type Foo
    x
end

Base.convert(::Type{Foo}, x, y) = Foo(x + y)

Foo(1) # works fine
Foo(1, 2, 3) # throws MethodError - should show the convert method

For some reason, calling Foo(1, 2) also throws a MethodError... I thought that was meant to fall back to convert ?

@KristofferC
Copy link
Member Author

KristofferC commented May 3, 2016

No it doesn't? Edit: On 0.5 it does throw! Seems like a regression, should probably open an issue for it.

julia> type Foo
           x
       end

julia> Base.convert(::Type{Foo}, x, y) = Foo(x + y)
convert (generic function with 536 methods)

julia> Foo(1, 2)
Foo(3)

@tkelman
Copy link
Contributor

tkelman commented May 3, 2016

sounds like #15120 ?

@Tetralux
Copy link
Contributor

Tetralux commented May 3, 2016

@tkelman Looks about right. Although, the number of fields doesn't seem to entirely matter; it doesn't work here either:

type Foo
  x
  y
end

Base.convert(::Type{Foo}, x, y, z, a) = Foo(x+y+z, x+y+z+a)

Foo(1, 2) # works fine
Foo(1, 2, 3, 4) # MethodError

It does appear to work, however, if the convert function takes exactly two arguments; the thing to convert to, and some value.

@yuyichao
Copy link
Contributor

yuyichao commented May 7, 2016

The fallback to convert is only for single argument constructor.

@vtjnash vtjnash merged commit b3d9db3 into JuliaLang:master May 10, 2016
@KristofferC KristofferC deleted the kc/closest_method_scoring branch May 10, 2016 18:20
@tkelman tkelman added the needs tests Unit tests are required for this change label May 10, 2016
@tkelman
Copy link
Contributor

tkelman commented May 10, 2016

could still use a test

@tkelman tkelman removed the needs tests Unit tests are required for this change label May 15, 2016
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.

5 participants