Skip to content

Completions for Prim#1092

Merged
kritzcreek merged 8 commits intomasterfrom
christoph/completions-for-prim
Jan 13, 2020
Merged

Completions for Prim#1092
kritzcreek merged 8 commits intomasterfrom
christoph/completions-for-prim

Conversation

@kritzcreek
Copy link
Contributor

This makes it so we complete functions and values from Prim at any location. I'll adjust this again once the Prim refactoring happened.

@nomeata
Copy link
Contributor

nomeata commented Jan 10, 2020

Are you confusing prim with prelude here? This seems to be dealing with what’s defined in the prelude.

After the refactoring in #1088 the prelude will only export types, but I don't expect any interactions with the LSP code.

|> Diag.map (fun (libs, _, scope) ->
let prim_defs =
Type.Env.fold(fun k v acc ->
if String.get k 0 = '@' then
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you could use Lib.String.chop_prefix "@" k != None to avoid hard-coding the lenght of @, but it’s not really shorter.

Or add Lib.String.has_prefix to lib.ml.

@nomeata
Copy link
Contributor

nomeata commented Jan 10, 2020

(Maybe the prelude will then gain definitions again, so this code is definitely useful – just not affected by the refactoring.)

@kritzcreek
Copy link
Contributor Author

kritzcreek commented Jan 10, 2020

Are you confusing prim with prelude here?

I probably do. After #1088 I'll just populate the IDE index with a synthetic path for the prim module (that's the one with the built-in functions, right?) and then things should continue working.

@nomeata
Copy link
Contributor

nomeata commented Jan 10, 2020

This makes it so we complete functions and values from Prim at any location.

When you say “any location”, what happens when you shadow a definition from the prelude in some scope?

@kritzcreek
Copy link
Contributor Author

If you shadow it with a top-level identifier (top-level meaning something at module scope) Things will work correctly for hovering. If you shadow it with a local let binding the IDE will show the wrong type.

For completions it's just going to suggest two completions with the same name but different types for a user-defined toplevel function and for local let bound things we're not showing any completions yet either way. I'm assuming most of this doesn't matter soonish as we'll have the Prim. prefix (or whatever the user chose) to disambiguate functions at the module scope from prim definitions.

@kritzcreek kritzcreek force-pushed the christoph/completions-for-prim branch from b5daaea to dbe8dde Compare January 13, 2020 12:43
@kritzcreek
Copy link
Contributor Author

The PR is a bit of a mess now because I fixed some stuff in the tests. Would you rather have me do that in a seperate PR or is it still small enough that you can look at it in one go @nomeata?

Co-Authored-By: Gabor Greif <gabor@dfinity.org>
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

LGTM!

@kritzcreek
Copy link
Contributor Author

Thanks @ggreif !

@kritzcreek kritzcreek merged commit d99b976 into master Jan 13, 2020
@kritzcreek kritzcreek deleted the christoph/completions-for-prim branch January 13, 2020 15:13
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