Improvments to REPL tab-completion (#3227) by rndnoise · Pull Request #3231 · purescript/purescript

rndnoise · 2018-01-31T03:42:01Z

Complete all names that have been imported (transitively or directly)
Do not complete names that haven't been imported
Only recompute list of names after import or adding a let binding
rather than after each request for name completion

This pull request fixes #3227

hdgarrood · 2018-01-31T16:30:43Z

src/Language/PureScript/Interactive/Types.hs


 -- * State helpers

+updateImportExports :: PSCiState -> PSCiState


Could you add a comment here saying something like

This function updates the Imports and Exports values in the PSCiState, which are used for handling completions. This function must be called whenever the PSCiState is modified to ensure that completions remain accurate.

Alternatively, is it worth using an explicit export list and hiding the PSCiState constructor so that we can easily verify that the PSCiState can't get out of sync?

Yeah, I think both are good ideas.

hdgarrood · 2018-01-31T16:30:54Z

src/Language/PureScript/Interactive/Types.hs


 initialPSCiState :: PSCiState
-initialPSCiState = PSCiState [] [] []
+initialPSCiState = updateImportExports (PSCiState [] [] [] nullImports nullExports)


Will updateImportExports do anything here?

Yes, perhaps surprisingly. It adds the types from Prim, which is implicitly imported in the module created in that function.

hdgarrood · 2018-01-31T16:37:24Z

tests/TestPsci/CompletionTest.hs

+  , ("G", ["GT"])
+  , ("P.G", ["P.GT"])
+  , ("P.uni", ["P.unit"])
+  , ("voi", []) -- import Prelude exclude (void)


The syntax is import Prelude hiding (void)

rndnoise · 2018-01-31T17:16:18Z

I've hidden the PSCiState constructor and added a comment to updateImportExports.

I also made an optimization to tests/TestPsci/CompletionTest.hs so the module is reused rather than setup and recompiled for each assertion (running the REPL tests was noticeably slow, it's much faster now). The remaining slow bit is tests/TestPsci/CommandTest.hs which I've left alone.

rndnoise · 2018-01-31T17:44:20Z

The build is failing due to an unused function allImportsOf. It's no longer used as the result of this PR, but I didn't want to delete someone's work without their permission. I think this code is yours, @hdgarrood ... should I remove it?

rndnoise · 2018-02-01T21:04:38Z

After a closer look, I don't think allImportsOf was useful outside of the older tab-completion code that called it; so I went ahead and removed it so the build would pass. Let me know if any further changes are needed before merging.

hdgarrood

Thanks for this, and the commit speeding up the REPL tests in particular looks great! However, it is really useful for us to be able to keep separate things in separate commits so that the history is easier to look over later, and also in case we ever need to revert one of the changes (but not the other). So would you mind splitting that off for now?

Removing any code which is now obsolete is definitely a good idea 👍

I just realised it would probably be helpful to add a comment near initialPSCiState to clarify the reason we need to use updateImportsExports there?

Also, looking at this I realised that we haven't actually managed to guarantee that the Imports and Exports stay in sync this way; if you export just the fields of a record type you're still able to modify the values that those fields have if you're given a value of that type. For instance, the change in app/Command/REPL.hs is changing the psciLoadedExterns directly, and not going via the utility functions exported by Types. I think we would need to convert PSCiState to a normal constructor (rather than using the record style) and define each of the accessors as normal functions rather than record fields to be able to guarantee that the state can only be modified in a certain way. But if doing it this way seems like it would be any more difficult than just a few mechanical translations then let's not worry about it for now.

hdgarrood · 2018-02-01T21:17:59Z

src/Language/PureScript/Interactive.hs

-  print' $ showModules importedModules
-  return ()
+  st <- get
+  print' $ showModules (psciImportedModules st)


Bit of a nitpick, but using $ as well as parens on the same line is stylistically perhaps not the best thing to do. I don't mind which but would you mind sticking to one or the other?

rndnoise · 2018-02-02T02:20:25Z

OK, I think I've addressed all of your points.

So would you mind splitting that off for now?

Sure, it is done. In general though, how do contributors here manage related or interdependent pull requests? Should those not be made into PRs until the dependency has been merged, or do people create PRs that have stacked on other PR branches?

I just realised it would probably be helpful to add a comment near initialPSCiState to clarify the reason we need to use updateImportsExports there?

I've removed that call and exported Sugar.Names.Env.primExports so that it could be used here instead of nullExports.

Also, looking at this I realised that we haven't actually managed to guarantee that the Imports and Exports stay in sync this way;

Good catch. I saw a compiler error halfway through making that change and assumed it confirmed my guess that I could export the type without exporting the constructor, but it must have been a different error. It is now a data declaration with some extra "getter" methods, so now it shouldn't be possible to take apart or build a PSCiState directly.

Bit of a nitpick, but using $ as well as parens on the same line is stylistically perhaps not the best thing to do

No problem.

kritzcreek · 2018-02-02T14:11:27Z

src/Language/PureScript/Interactive/Types.hs

-  , psciLoadedExterns       :: [(P.Module, P.ExternsFile)]
-  } deriving Show
+  [ImportedModule]            -- ^ psciImportedModules
+  [P.Declaration]             -- ^ psciLetBindings


Haddock is crashing on this line. It doesn't seem like you're allowed to annotate the individual arguments to a constructor, just the whole constructor or record fields: https://www.haskell.org/haddock/doc/html/ch03s02.html

hdgarrood

In general though, how do contributors here manage related or interdependent pull requests? Should those not be made into PRs until the dependency has been merged, or do people create PRs that have stacked on other PR branches?

I've seen both approaches used and they both seem to work fine. I think if you open a new PR which depends on some other still-open PR, we'd probably ask you to rebase it after we've merged the first one.

hdgarrood · 2018-02-02T14:49:09Z

src/Language/PureScript/Interactive/Types.hs

+module Language.PureScript.Interactive.Types
+  ( PSCiConfig(..)
+  , PSCiState -- constructor is not exported, to prevent psciImports and psciExports from
+              -- becoming inconsistent with importedModules, letBindnigs and loadedExterns


typo here? "letBindnigs"

Yes, thanks :-)

hdgarrood · 2018-02-02T14:52:49Z

This looks great; I'm happy to merge once CI is passing.

- Complete all names that have been imported (transitively or directly) - Do not complete names that haven't been imported - Only recompute list of names after import or adding a let binding rather than after each request for name completion This commit fixes #3227

hdgarrood

Great, thanks very much!

hdgarrood reviewed Jan 31, 2018

View reviewed changes

rndnoise mentioned this pull request Feb 1, 2018

REPL doesn't auto-complete re-exported symbols #3227

Closed

hdgarrood reviewed Feb 1, 2018

View reviewed changes

kritzcreek reviewed Feb 2, 2018

View reviewed changes

hdgarrood reviewed Feb 2, 2018

View reviewed changes

This was referenced Feb 2, 2018

Complete after parens and brackets in REPL #3236

Merged

Complete any number of words after :kind and :type in REPL #3237

Merged

hdgarrood approved these changes Feb 2, 2018

View reviewed changes

hdgarrood merged commit f999179 into purescript:master Feb 2, 2018

hdgarrood mentioned this pull request Feb 2, 2018

PSCi re-export handling needs some work #2080

Closed


		-- * State helpers

		updateImportExports :: PSCiState -> PSCiState

Conversation

rndnoise commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rndnoise commented Jan 31, 2018

Uh oh!

rndnoise commented Jan 31, 2018

Uh oh!

rndnoise commented Feb 1, 2018

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rndnoise commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented Feb 2, 2018

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants