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

Use type inference to determine the call signature for method completion. #11679

Merged
merged 1 commit into from
Nov 8, 2015

Conversation

dhoegh
Copy link
Contributor

@dhoegh dhoegh commented Jun 12, 2015

This pull implement method completion using type inference to determine the input types for a function call, Second case in #6338. This means the method completion suggestion removes the methods not appropriate for the function call, see:

julia> dot([1],<tab>
dot(x::AbstractArray{T,1}, y::AbstractArray{T,1}) at linalg/generic.jl:280
julia> dot(max(1,1),<tab>
dot(x::Number, y::Number) at linalg/generic.jl:279

The only thing I do not like about the implementation, is the need for eval an expression of the arguments into an anonymous function, see line:

afunc=eval(Expr(:->, Expr(:tuple, key_s...), ex_org.args[i]))
. I do only let type inference work its magic on the anonymous function so the anonymous function is never called, so it might be ok? This might not be necessary if I could create the LambdaStaticData manually for the anonymous function, anyone know how this could be done?
It only analyse function arguments that is a call or is lowered to a call. Hence this do not work when the arguments is an expression. Example the time macro return an expression. I have also added this as a test case in https://github.com/dhoegh/julia/blob/603d16c7742fed2b10106402ec7284cf0e7c1f6e/test/replcompletions.jl#L292.
cc @blakejohnson could you review this, you reviewed similar additions in #9676?

Further work on this pull includes

  • Make tests

@tkelman tkelman added the REPL Julia's REPL (Read Eval Print Loop) label Jun 12, 2015
@@ -203,35 +203,77 @@ function find_start_brace(s::AbstractString)
end

# Returns the value in a expression if sym is defined in current namespace fn.
# The vars argument is a dictionary that contains all variables in the ex_org
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing whitespace

@dhoegh
Copy link
Contributor Author

dhoegh commented Jun 13, 2015

@tkelman thank you for the reminder, I have pushed a new version with fixed whitespaces and added test cases.

@blakejohnson
Copy link
Contributor

I'll try to take a look at this later this weekend.

@dhoegh
Copy link
Contributor Author

dhoegh commented Jun 13, 2015

Thank you @blakejohnson, the Travis/Appveyor failures are unrelated.

@dhoegh
Copy link
Contributor Author

dhoegh commented Jun 14, 2015

@blakejohnson, the code is not versatile enough to handle all cases, so hold off reviewing this.
I need to figure out how to use the inference in a more generic way. I would like to tell the inference to assume that the global variables in the function do not change type.

@dhoegh dhoegh force-pushed the method_comp_inference branch 4 times, most recently from 6b13082 to 603d16c Compare October 13, 2015 17:19
@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 13, 2015

I finally got around to rebase and solve this PR in a better way. @blakejohnson Could you review this?

@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 24, 2015

Bump, anyone willing to review.

@blakejohnson
Copy link
Contributor

Sorry, if no one gets to it before me, I'll try to have a look early this week.

@dhoegh
Copy link
Contributor Author

dhoegh commented Nov 7, 2015

Bump.

@blakejohnson
Copy link
Contributor

So, my reading of this change is that it appears correct. I always have some trouble reading code that does expression parsing, though, because it is never particularly clear what you are extracting in statements like ex.args[2].

How do people feel about REPL completion behavior depending on type inference? There has previously been resistance to making program behavior depend on inference. This seems like a different category, though, since the programs you write aren't changed, but only the help you receive while writing them.

@@ -222,14 +222,61 @@ get_value(sym::Symbol, fn) = isdefined(fn, sym) ? (fn.(sym), true) : (nothing, f
get_value(sym::QuoteNode, fn) = isdefined(fn, sym.value) ? (fn.(sym.value), true) : (nothing, false)
get_value(sym, fn) = sym, true

# Return the value of a getfield call expression
function get_value_getfield(ex::Expr, fn)
val, found = get_value_getfield(ex.args[2],fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we help the reader with a comment about what is contained in ex.args[2] and ex.args[3]?

@blakejohnson
Copy link
Contributor

Thank you, @dhoegh, for including some thorough tests with this change.

@tkelman
Copy link
Contributor

tkelman commented Nov 8, 2015

Since this is interactive, the inference issue seems less critical here. Though hypothetically could user programs hook into this and try to use it for other purposes? Maybe not worth worrying about.

@dhoegh
Copy link
Contributor Author

dhoegh commented Nov 8, 2015

So, my reading of this change is that it appears correct. I always have some trouble reading code that does expression parsing, though, because it is never particularly clear what you are extracting in statements like ex.args[2].

I have added further comments, to try to clarify what is happening.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 8, 2015

Though hypothetically could user programs hook into this and try to use it for other purposes?

It can always call type inference directly, e.g. code_typed, return_types etc, so there's nothing to worry about type-inference dependence there.

@tkelman
Copy link
Contributor

tkelman commented Nov 8, 2015

Ah yeah, duh. Nevermind then.

This looks okay to me, shall we merge?

@blakejohnson
Copy link
Contributor

Yes, I think this is good to merge.
On Sun, Nov 8, 2015 at 8:38 AM Tony Kelman [email protected] wrote:

Ah yeah, duh. Nevermind then.

This looks okay to me, shall we merge?


Reply to this email directly or view it on GitHub
#11679 (comment).

…ove non matching methods. Add test for the new functionality, fix  JuliaLang#6338.
@dhoegh
Copy link
Contributor Author

dhoegh commented Nov 8, 2015

I have just squashed the commits, so this should be good to merge.

blakejohnson added a commit that referenced this pull request Nov 8, 2015
Use type inference to determine the call signature for method completion.
@blakejohnson blakejohnson merged commit e9af3f6 into JuliaLang:master Nov 8, 2015
@KristofferC
Copy link
Member

Cool, this is a nice thing to have.

@dhoegh
Copy link
Contributor Author

dhoegh commented Nov 8, 2015

Thank you, @blakejohnson, for the review and merge.

tkelman pushed a commit that referenced this pull request Nov 29, 2015
…ove non matching methods. Add test for the new functionality, fix  #6338.

(cherry picked from commit f8196cc)
ref #11679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants