Completion: Fixed

[jdtsmith]

I fixed the issues with PR #119:

• The require tests didn't have the library name correct, and in any case LUA_PATH needed to be set so require could find the library to begin with.
• All but the last line of multi-line expressions were being dropped by the completion engine (despite the mode correctly identifying the full expression).

So e.g.:

table.
   ins[M-Tab]

couldn't correctly complete.

I also switched sed to perl in the Makefile since sed command switches aren't portable.

Longer term, switching to comint-simple-send with comint-preoutput-filter-functions to hide the back channel completion communication with the lua process would be preferable to the current temp file write/read. That's pretty easy to implement so I may have a go at that.

It also seems very inefficient to clone the full nested _G var each and every time completion occurs. Probably better to serialize the full completion tree into Elisp as needed, and then provide a user command to update it on demand. That said, performance was fine for me (using a small embedded Lua inside of Hammerspoon). Large package sets and more active completes like company-mode could start to impact performance.

Recognizing a few more cases of library load, and local aliasing, e.g.:

local hasLib, lib = pcall(require,"lib") 
local ls=lib.sublib

would also be useful.

[immerrr]

Great stuff. Travis still fails for some reason, do you know why?

I'll try to have a closer look at the changes over the weekend.

[jdtsmith]

I am now "debugging" Travis on a separate branch, since all buttercup tests succeed on my machine. It is almost certainly due to the need for asynchronous process communication (i.e. "wait for the prompt before doing anything else"), which right now is not being implemented. That leads to platform/machine/lua-version/test-specific brokenness. Sigh. I'll probably have to bite the bullet and implement a simple comint-output-filter-functions callback. BTW, it might be helpful to drop support for 24.3 and prior. Let me know if that's a concern.

[immerrr]

I am not a big fan of dropping compatibility unnecessarily, as I myself had quite a bit of pain resolving dependencies for some packages while living on few-years-old Ubuntu LTS releases. But seeing that Debian Stable/Ubuntu 14.04 both have emacs=24.5, I guess dropping 24.3 and older should probably be fine.

[immerrr]

Hmm, I'm seeing the same error when trying to run the tests locally, let me have a closer look...

[immerrr]

Welp, so much for the debugging opportunity. Now the tests are working, and I didn't even do anything 🤷‍♂️

[jdtsmith]

Yeah it's all related to the fact that the completion framework has no asynchronous mode or callback, so we always need to block and check for the prompt before proceeding. Sometimes you get lucky timing-wise, other times not. Spent way too much time digging into this this week. I have substantially rewritten much of the completion code to avoid the use an external file, using comint-redirect-send-command-to-process and a scratch buffer instead. Once I finish debugging I'll push those commits and have you check on your end.

[immerrr]

I am not entirely sure it's related to timing, could be some sort of misconfiguration.

I wonder if the tests could be improved by checking the output of lua-complete-function instead of the result of the entire end-to-end completion framework processing. We could check the entire completion list and maybe distinguish between the helper script having created the output file or not. Or even have the helper return the strings as JSON or a read-able sexp, so that we have a brace/parenthesis at the end of output clearly indicating that there will be no data afterwards.

Let me try that...

[jdtsmith]

I wouldn't suggest putting too much effort into the current version on this branch as I've made some substantial changes and don't want to have conflicts. In fact let me just organize and commit my current changes so you can play as well.

[jdtsmith]

I export the LUA_PATH variable in the Makefile so the library require tests work.
I think it would be better to tweak process-environment variable directly in the related test, so that it is not only when you run it via Makefile that the environment gets configured correctly.

That sounds very simple and easy to implement. I agree a future PR might be the right place for this simple change.

[jdtsmith]

A number of the completion tests were broken and malformed. For example, before-all was used with with-lua-buffer, which of course kills the buffer before any tests run! I ended up with a few :var variables to run the tests. Again, you have to have lua-process-buffer current to access all the buffer-local variables. This is a crucial point probably worth mentioning in the file somewhere.

That was actually intentional. The idea was to have lua--get-completions function to run independently of lua-mode being activated, so that a) the tests run faster as there is less activations/deactivations in the way and b) the scope of the test is smaller, and if something fails you know it is rather about the lua--get-completions function itself rather than the surrounding code. Although admittedly, at this point you have much more knowledge about how this is supposed to work, so it might have been misconceived from the very beginning.

If you want to check that the require library scanning works without a process that's fine, but all the xyz.* tests require a running process. So of course they would fail as you spun up a process, killed it, and then queried the shell for keyword completions of an externally required local variable.

[jdtsmith]

deleted buffer was actually a much deeper issue: make-comint recycles the same buffer, so we need a unique name.

Do we need a unique name if we create a buffer ourselves and then use make-comint-in-buffer on that?

Per-lua-buffer unique is what I mean. When setting to buffer-local variables, it wasn't enough to just make-comint with a buffer-local lua-process-buffer, since it would recycle the same buffer. So now it appends the buffer name to *lua* to avoid that. Or did you mean something different?

[jdtsmith]

Re: caching and the simpler TODO's, let's postpone them until the main part of this improvement is checked in.

Agreed. I actually implemented a draft version of "return the list of everything below here", in e.g. tabl[M-Tab]. It was very simple (mostly just changes to the lua code that runs), and it works great with various completion styles. It also allows you to use completion-table-with-cache, which reduces trips out to the lua process. The problem I encountered is completing really deep and wide trees. An example is the internal Lua in Hammerspoon, which has a large hs object that goes very deep. Force completing e.g. hs.[M-Tab] stalls out pretty bad. An idea to fix it is to cut the tree descent in lua after some fixed number of items have been discovered (say at 500 or 1000), performing a breadth-first search at the entry level (trivial), or using a separate queue to depth-first at all levels of the tree, so as to keep the 500 or 1000 completion items as close as possible to the top of the tree. The you can use with-cache style caching, and if the cached version comes up short, re-query lua. So sort of a hybrid cache with the best of all worlds.

That would require writing our own version of a completion-table-with-cache style wrapper that re-tries lua if the cache fails. Small changes, but quite subtle, so definitely another PR. It might be simpler to use lexical binding and closures to implement this (as comint does), so perhaps another small TODO is to convert Lua-mode to lexical binding (which offers speed improvements that will grow with time).

The changes are quite massive, so I'd propose a plan: let's go over the pending Q&A's, I'll try to go over the changes once again in the coming days to see if there are any unasked Qs left, then we see if we can rewrite the changes into a more sizeable series of commits without losing generality, and then finally check it in to master and then the QoL improvements can come incrementally from that point on.

Would it work for you?

Sounds good. Take a closer look and ideally play with completion in your own environment.

The commit tree on this PR branch got a bit trampled by having to retool to sort out CI issues, so I agree we probably shouldn't expose all that to master. I suppose PR branches get kept around for posterity, in case anyone wants to recreate the two-steps-forward-one-step-back progress?

[jdtsmith]

One other small improvement I should probably port onto this version. Some embedded Lua's don't implement io.write in a useful way (e.g. the Hammerspoon lua I mentioned above). So I have a small change that uses print() instead of io.write, but it (for some Lua's) outputs ANSI color codes, which I need to strip out. Let me know if you mind if I add those changes.

[jdtsmith]

You got me to think hard about buffer-tied Lua processes, so rather than ask the user to rig that themselves, I added a lua-process-is-buffer-local customize which sets everything up for this (and I use that for the tests).

Very important is that the lua-process-buffer holds all the correct buffer-local variables (like output buffer, etc.), so it's important to with-current-buffer that when accessing them. When this option set, the per-lua file process buffer is lua [buffername]. In principle you could let-bind lua-process-is-buffer-local for a single buffer and let all other lua buffers share a process.

Would it make the workflow simpler to understand if we took it one step further and made shell-specific variables, like output-buffer and last-command, and even lua-process variable itself, always buffer-local and always configured in the corresponding lua-process-buffer?

I do like this. I felt like coordinating all the relevant variables which are process-centric with lua-process-buffer is the preferred setup, that way they go down with the process buffer when it closes. Probably even lua-process shouldn't be tracked separately, since a process can be destroyed out of band with you knowing it, and it's easy to ask the buffer for its process (Stefan Monnier recommended that for major modes). Since I found almost nowhere that lua-process was used, I eliminated it; take a look.

We could drop the is-local configuration flag and then whether or not lua-process-buffer would be buffer-local or not, the rest of the functionality would be exactly the same. Basically, the only action necessary to reassociate a lua-mode buffer to a different inferior process would be to change the lua-process-buffer variable. We could even plug in a 3rdparty inferior process management library to have more flexibility about the buffer-inferior buffer relations, smth like having one inferior shell per projectile project with lua-path customizations and whatnot, and then have all buffers in that project could share it.

Good points. I'd definitely be annoyed to have per-buffer processes by default, so I don't think that's a good option. But I see your point; in a separate commit, I changed lua-setup-local-variables to always attach the process-relevant variables to the lua-process-buffer, relegating lua-process-is-buffer-local mostly to simply an explicit way to set lua-process-buffer buffer-local. So we could even eliminate this variable, though I don't think it does any harm if someone uses a 3rd party library to tie buffer-locality to project or some other construct (which I'd never heard of). Right now the variable is still needed to create a different process-buffer name per buffer, but maybe that's something these libraries do as well? But obviously I could just check if it's buffer-local instead and change the name accordingly. If you don't change the process-buffer name, by default make-comint will just recycle the same process in the same buffer (which surprised me). So I'm not sure how those libraries do their magic...

Have a look.

[immerrr]

by default make-comint will just recycle the same process in the same buffer (which surprised me)

Buffer reuse can probably be fixed by replacing (UPD: sorry, let me rephrase that)
When lua-process-buffer is nil we can probably guarantee that we are creating a new buffer with a new process by replacing

(make-comint name program startfile switches)

with smth like

(setq lua-process-buffer (generate-new-buffer (concat "*" name "*")))
(make-comint-in-buffer name lua-process-buffer program startfile switches)

[immerrr]

I'd definitely be annoyed to have per-buffer processes by default, so I don't think that's a good option.

Agreed, the buffers should be shared by default by the normal sharing of global variables between buffers. If we come across a more sophisticated inferior process sharing that makes the variable buffer local and then sets the buffer-local values applying some domain knowledge to figure out which processes must be kept separate, but the default behaviour should be to share anyway.

Have a look.

Looks good, but probably a better fit would be the make-variable-buffer-local function run once right after the corresponding variable declarations.

[jdtsmith]

by default make-comint will just recycle the same process in the same buffer (which surprised me)

Buffer reuse can probably be fixed by replacing (UPD: sorry, let me rephrase that)
When lua-process-buffer is nil we can probably guarantee that we are creating a new buffer with a new process by replacing

(make-comint name program startfile switches)

with smth like

(setq lua-process-buffer (generate-new-buffer (concat "*" name "*")))
(make-comint-in-buffer name lua-process-buffer program startfile switches)

That's effectively what I'm doing, but using buffer name to disambiguate. Maybe that's not the right choice, if we want something like "these 3 buffers share one lua process, the rest share another" or similar, i.e. for these process managers you allude to.

One more issue: if you re-call start-process from the same buffer with buffer-local lua-process-buffer, this would always create another new process. We do want some degree of process-buffer recycling (when an associated process is running). I suppose we could merge the logic of lua-get-create-process into lua-start-process and eliminate the latter (or just rename them lua-start-process and lua-start-process-new). Then you could run-lua anywhere with impunity.

[jdtsmith]

I'd definitely be annoyed to have per-buffer processes by default, so I don't think that's a good option.

Agreed, the buffers should be shared by default by the normal sharing of global variables between buffers. If we come across a more sophisticated inferior process sharing that makes the variable buffer local and then sets the buffer-local values applying some domain knowledge to figure out which processes must be kept separate, but the default behaviour should be to share anyway.

Right. Though it does seem that these process sharing managers will need a way to affect the process buffer names.

Have a look.

Looks good, but probably a better fit would be the make-variable-buffer-local function run once right after the corresponding variable declarations.

I see, so they are always buffer-local. OK, you convinced me. See the latest commit. I removed the local variable setup code entirely, favoring defvar-local for all the relevant variables. And to avoid always creating a new buffer, I'm combining the logic of lua-get-create-process with lua-start-process, making the former an alias to the latter. So that way you always get a running process back. And if you have an extant buffer in lua-process-buffer, it will reuse that buffer and (if needed) re-start a new process.

One way to "manage" process buffer-locality would then be to add a file local variable, ala:

--*- lua-process-buffer: nil -*-

This isn't as grok'able as

--*- lua-process-buffer-is-buffer-local: t -*-

But it works. And if we accept any pre-set lua-process-buffer name (not just a live buffer), you could also then set a directory-local variable and give lua-process-buffer a fixed name, ala:

--*- lua-process-buffer: "*lua*<MyProject>" -*-

constituting a poor-man's form of process management.

[jdtsmith]

Alright, I think I'm converging on a stable PR here. I made a number of small changes in the last commit batch. I try to avoid gratuitous with-current-buffer's all over the place since they are expensive, instead defining a lua-proc-get|set for accessing the buffer local vars in the process buffer. All this per-process stuff requires care, since sometimes unexpected variables are process buffer local (like the all-done flag comint-redirect-completed). I can verify this works well with a directory local setting of lua-process-buffer to a string to share the same process amongst all .lua files beneath a given directory, or nil for a per-file/buffer lua process. Pretty convenient to load up many files that share a lua process, but then have another (perhaps global) process for other files.

Also, I did go ahead and switch to print (and found a way to make my troublesome embedded lua stop outputting color codes).

I am considering whether to put effort into making completion work in the shell here, or in a later PR. It's a trivial addition, but the biggest issue is the process buffer doesn't know its own name. I could make the process buffer know lua-process-buffer, but then I'd have to keep that in sync with the originating .lua buffer. Or I could always check if I'm in a process buffer and use that preferentially.

[jdtsmith]

Not sure if you are still digging around in this PR. I have another branch which picks up on this work and implements some new functionality, but didn't want to distract you if you were in the midst of review: see my branch completion-with-cache.

In my new branch (which just builds on this PR branch), I've implemented completion as a queue-based breadth-first search of the lua global tree, returning all possible completions below the point, independent of depth. So e.g. tabl[M-Tab] produces table|table.concat|table.insert|table.move|table.pack|table.remove|table.sort|table.unpack. It's limited by default to 1000 entries (which takes ~1/4s for me; this is actually enough to traverse the entire large table structure I mentioned earlier, now that I trim tables beneath underscore _keys).

This required fully rewriting the lua code lua-mode sends to the process. The new lua function __emacs_lua_complete in a standalone file is now compiled when the shell initializes, and is used to generate completions. As a bonus this is much faster than the old setup.

My testing showed that the completion framework calls back 6 or more times per completion action. Originally, each and every time, we'd hit lua up for the same completion list. So I also implemented a custom lua-sorted-completion-table for completion-at-point-functions.

This replaces completion-table-dynamic/etc. It "soft"-caches the completion results from the lua process, re-querying lua if the cache misses (just in case 1000 entries wasn't enough). It also sorts its completion results such that those with the fewest dots come up first. Even if you artificially limit yourself to say 30 entries, it's still quite usable, as the mostly likely completions come up first, and you can easily re-query as you go deeper and the tree thins out.

I'm pretty well done modulo any issues you spot (I'm going to leave the shell alone for now). Options for this:

1. We can merge my completion-with-cache branch into the current PR branch and proceed as you proposed earlier.
2. We can get this PR applied, then I can submit the new work as a new PR to build on top of it.

I favor option 1, since the new branch is just far more usable, never bogs down with company mode, etc. Let me know how you want to proceed.

PS: One other oddity I found that explains much of the failing-travis stuff: in clean interactive emacs sessions (like make tryout), process PTY's don't echo or convert newlines to CRNL; in non-interactive emacs runs (make test) the PTY's were by default doing both. No idea why. Now I explicitly set the tty settings when lua starts.

[jdtsmith]

To make it simpler for you to review, I condensed history into many fewer commits and opened a new PR: #148. That PR supersedes this one. If you agree, feel free to close this one.