Skip to content

Type search #55

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

Merged
merged 2 commits into from
Jan 16, 2017
Merged

Type search #55

merged 2 commits into from
Jan 16, 2017

Conversation

paf31
Copy link
Contributor

@paf31 paf31 commented Jan 14, 2017

This is a work-in-progress.

The idea is to expose a /search endpoint which Pursuit can use to implement type search using the externs held by the Try PureScript process.

I added this here instead of in a separate service since we already have the data in memory, and it seems wasteful to load it all twice on the server when we have limited resources.

The main issue is that replacing type variables with unknowns generally yields too many results. We could use skolems instead, which works better in some cases, but in other cases it gives too few results, since the generalization step doesn't work in the solver.

Another thing I would like to try is to resolve unqualified constructors by looking up any matching constructors in the Environment.

@paf31
Copy link
Contributor Author

paf31 commented Jan 14, 2017

Ok, I think this is ready to review now.

@hdgarrood We can probably use this to evaluate the quality of the search results that we'd be able to get from package set based search, to help decide whether it's worth the effort of maintaning package sets more generally.

cc @soupi

Copy link
Collaborator

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Bit of a nit but perhaps the URL path should be eg /type-search to make it clear that this is only searching by type?

I don't think I know enough just yet to be able to help with issues like skolems vs unknowns.

flexMatches = search (replaceTypeVariablesAndDesugar (const P.TUnknown) elab)
take 50 (strictMatches ++ flexMatches)
Scotty.json $ A.object [ "results" .= [ P.showQualified P.runIdent k
| (k, _) <- take 50 results
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are doing take 50 twice? (Line 113)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's deliberate. The idea is to explore the search space lazily for every combination of type constructors with the specified names. I want the nested searches to be bounded, but also the top-level search itself. Really I should probably use LogicT or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. Of course :)

@soupi
Copy link
Contributor

soupi commented Jan 15, 2017

This looks good to me but i'll have to take it for a spin to really understand what's going on. will try to do it later today :)

@hdgarrood
Copy link
Collaborator

Also, while I'm not sure how much effort it will be to maintain package sets, my expectation is that it will provide a huge benefit in helping people find install plans; even that on its own, without nice extras such as this type search, could well pay for itself I think. Having used Stackage I'm pretty confident that package sets are a good idea. Of course we're not quite at a stage where our tools can easily make use of package sets to help find install plans, but I do want to look at that at some point.

@paf31 paf31 merged commit d7b12be into master Jan 16, 2017
@paf31 paf31 deleted the phil/type-search branch January 16, 2017 22:25
@paf31
Copy link
Contributor Author

paf31 commented Jan 16, 2017

I've deployed a first version, and you can try it here:

@hdgarrood Sorry, I missed your comment about the route being /type-search. But this is only meant to be temporary anyway. If we integrate it into Pursuit, we can always come back and fix it up.

@hdgarrood
Copy link
Collaborator

Nice! Can we do anything to move functions like unsafeCoerce a little further down in the list (do we have an issue forthat)? Also, it has only just occurred to me that this would be a bit more useful to Pursuit if we included the package names as well.

@paf31
Copy link
Contributor Author

paf31 commented Jan 16, 2017

Can we do anything to move functions like unsafeCoerce a little further down in the list (do we have an issue forthat)?

There's no simple way that I can think of, although Pursuit does have some code to estimate the number of instantiations required to unify two types. Perhaps we could reuse that?

this would be a bit more useful to Pursuit if we included the package names as well.

Oh right, of course. We don't have that information in Try PureScript right now unfortunately. Like the compiler, it just receives a glob of source files on the command line.

@hdgarrood
Copy link
Collaborator

I was thinking of something like that, yeah. Might it make sense to put something like that into the compiler so that its suggestions based on this type search can benefit from that too?

Ah ok, if that information isn't easily available then let's not worry about it for now.

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.

3 participants