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

Add some inference based on use of getfield. #134

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Apr 20, 2020

This adds another inference pass to non-toplevel scopes. Type information is added for bindings where getfield is used on the variable through the dot operator.

struct T
    somefieldname1
end
struct S
    somefieldname1
    somefieldname2
end
function f(arg1, arg2, arg3)
   arg1.somefieldname1 + arg1.somefieldname2
   arg2.somefieldname2
   arg3.somefieldname1
end

In the above example arg1 and arg2 would be inferred as S (arg3 could not be inferred due to somefieldname1 exists across both types. A map of fieldnames-to-datatype for the symbolserver cache is stored. Repeated construction of the equivalent map for the top-level scope can be improved on.

This is a considerably different approach to how we currently do our limited inference (i.e. its a bit more speculative) so is worthy of discussion before I get too involved in exploring this.

@davidanthoff
Copy link
Member

Hm, I have to admit I don't understand this :) Is the idea that it essentially does a pattern matching, i.e. sees that certain fields are used inside the function, and then it looks for all types that have these fields, and if it finds only one, it assumes that must be the type?

Isn't that quite brittle, in particular because we don't have access to the universe of types that could be passed?

Another question: how does this interact with getproperty?

@ZacLN
Copy link
Contributor Author

ZacLN commented Apr 21, 2020

It's potentially quite brittle but I think this very much depends on how this type info is used (currently it just supports the helpers - there's no linting based on it). We do have the universe of types and their fields (part of the reason for the overhaul of SymbolServer) so I don't see that as being too much of a problem

The getproperty issue is the one I'm concerned about - it's one of those features that means you have to throw out a lot of the assumptions you'd like to make about a users code. I'm not sure how widely it's used in practice (23 methods in a fresh REPL and an additional 19 if I load all ~ 200 packages in my env's manifest). This is the issue I'd look into next

@davidanthoff
Copy link
Member

currently it just supports the helpers

What are those? Man, I really don't understand the LS ;)

We do have the universe of types and their fields

But say I'm working on a package, then a user can use that package in an env with completely different packages and types that we (nor the user editing a package) knows about, right?

@ZacLN
Copy link
Contributor Author

ZacLN commented Apr 21, 2020

By helpers I just mean things like completions/hovers etc - I've not set this up to do anything like say "this function call is wrong because the argument isn't of the correct type"

But say I'm working on a package, then a user can use that package in an env with completely different packages and types that we (nor the user editing a package) knows about, right?

Yes, so there would be a different inference result depending on what packages are usinged in your current project which is what you'd want/expect.

@ZacLN
Copy link
Contributor Author

ZacLN commented Apr 26, 2020

I've added a bit more to this - a similar approach based on what functions are called with the binding as an argument. I think this is an interesting approach to explore and could be pretty effective but will take a bit of time to get right. Ultimately we should be able to detect otherwise hard to spot errors where a variable is used as two things incorrectly - this would be a great feature.

There's a bit of a ways to go before that, but I'd rather get this into master (and the parallel PR for LanguageServer) now while the PR is relatively small and its impact is fairly minimal.

davidanthoff
davidanthoff previously approved these changes Apr 26, 2020
Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

I have to admit I'm still not entirely sold, but I'm happy to go along and give it a try :)


Assumes `func` is the definition of a function for `get_property`. Searches for
comparisons within the body between the second argument of the function and
symbols, returning a list of these symbols.
Copy link
Member

Choose a reason for hiding this comment

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

Is that a common pattern? The cases of getproperty that I'm familiar with almost all look in a dict, or some type parameters or something like that for names...

@davidanthoff
Copy link
Member

I'm putting this on the backlog for now, I think we don't want to try this before Juliacon, right?

@davidanthoff davidanthoff modified the milestones: Next extension patch release, Backlog Jun 18, 2020
@davidanthoff davidanthoff modified the milestones: Backlog, Triage Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants