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

Completion caching (simplified) #148

Closed
wants to merge 25 commits into from

Conversation

jdtsmith
Copy link

@jdtsmith jdtsmith commented May 24, 2019

This supersedes #147. It is simply a cleaned-up and much-simplified history of all the cumulative changes from #119 and the later additions of breadth-first globals tree traversal, caching, and comint-redirect interface with the lua process begun in #147.

I did not document the ability to make lua-process-buffer buffer-local, which might be a nice addition.

@jdtsmith jdtsmith mentioned this pull request May 24, 2019
-- Written 2019/05 J.D. Smith
function __emacs_lua_complete(parts,libs,locals,limit)
-- Setup a FIFO queue for breadth-first tree traversal
local queue={head=0,tail=0}
Copy link
Owner

Choose a reason for hiding this comment

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

Coming from Python I feel like this line is missing whitespaces around equals signs (or binary operators). What are the common style guidelines in Lua world? I saw some packages defining whitespace requirements, and some just deferring to "whatever is the default in PIL".

Copy link
Author

Choose a reason for hiding this comment

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

lua is very lenient about whitespace in either direction, but I can certainly add it in for visual clarity. Added. BTW, I will alter history in this branch after the fact to avoid the clutter of small commits like these, so please don't rely on specific commit ID's!

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, let's please add whitespace around binary operators and newlines where necessary, now that there's no necessity to keep the size to the minimum.

And things like

    local globals,cnt,limit={},0,limit or 2000 

could probably use a touch to clarify (limit assignment in particular catches me off guard every time I return to this piece of code).

@@ -0,0 +1,81 @@
-- __emacs_lua_complete:
Copy link
Owner

Choose a reason for hiding this comment

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

Nice step to move this away, it is clearly way too complicated to include in lua-mode.el file verbatim. One extra thing to do here is to figure out the packaging part. For example, we could put this into scripts/ directory, and update MELPA recipe and the Cask file to include (:files :defaults ("scripts", "*")).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. It also saves constant recompilation every time you complete, and opens the door for more "behind-the-scenes" interaction with lua going forward.

The scripts sounds like a good plan. I created a scripts directory and referenced it. I'm not really familiar with Cask; I think you have push access to this branch, if you don't mind sending a commit over.


-- Descend the tree to the limit of dotted parts
local g,pre=globals,""
for i=1,#parts do
Copy link
Owner

Choose a reason for hiding this comment

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

Would ipairs work here to replace all the parts[i] in the body of the loop?

Copy link
Author

Choose a reason for hiding this comment

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

Well, I use i to tell me if I'm in the "last" part, which may be a partial match (or even an empty string for table.[M-Tab] type completions). But I'm by no means a lua expert, so let me know if there's a better lua way.

Copy link
Owner

@immerrr immerrr Jun 14, 2019

Choose a reason for hiding this comment

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

No, what I'm saying is that you are basically doing

for i = 1, #parts do
    current_part = parts[i]

whereas ipairs will do that for you automatically

for i, current_part in ipairs(parts)

and you get to keep both i and current_part in the loop's body.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, fixed.

emacs_lua_complete.lua Outdated Show resolved Hide resolved
(require 'cl))

(require 'cl)
(require 'subr-x))
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably add (emacs "24.4") to package dependencies in Cask (purcell/package-lint#110 for reference)

Copy link
Author

Choose a reason for hiding this comment

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

Right. Again I'm not a Cask expert; would you mind adding the correct incantations and submitting to to this PR branch? I'll stop rewriting pushed history until we converge. Anything that needs to be placed elsewhere just add a [TOSQUASH].

@@ -304,7 +315,7 @@ If the latter is nil, the keymap translates into `lua-mode-map' verbatim.")
key like `{' is pressed")
(make-variable-buffer-local 'lua-electric-flag)

(defcustom lua-prompt-regexp "[^\n]*\\(>[\t ]+\\)+$"
(defcustom lua-prompt-regexp "^\\(>[\t ]+\\)+"
Copy link
Owner

Choose a reason for hiding this comment

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

Will this regexp break for code that does io.write and doesn't append a newline? E.g.

$ lua
Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
> io.write('asdf')
asdf> 

Copy link
Author

Choose a reason for hiding this comment

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

I hadn't thought of that. Interestingly, for me I get:

Lua 5.3.5  Copyright (C) 1994-2018 Lua.org, PUC-Rio
No entry for terminal type "emacs";
using dumb terminal settings.
> io.write('asdf')
asdffile (0x7fff9d8f21a8)
> 

But the old version will fail for prompts with more information after them, as in the Shell itself. We have to tie either to the bol or eol; most comint-major modes with shells choose the former.

Copy link
Owner

Choose a reason for hiding this comment

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

My version does

> io.write("asdf")
asdf> io.write("zzz")
zzz> 

It should be fine as the first version with a TODO/FIXME, but I don't quite like that there is an infinite loop

      (while (not comint-redirect-completed)
	(accept-process-output process))

that depends on this regexp. Let's get this in and then think if there is a more reliable way to indicate the end.

lua-mode.el Show resolved Hide resolved
@immerrr
Copy link
Owner

immerrr commented May 25, 2019

Hopefully, one last round of questions. I am sorry it takes forever, but it is just that every rewrite or functionality change has the potential of generating more questions and doubts. Smaller changes are much easier and faster to get in.

@jdtsmith
Copy link
Author

Hopefully, one last round of questions. I am sorry it takes forever, but it is just that every rewrite or functionality change has the potential of generating more questions and doubts. Smaller changes are much easier and faster to get in.

No worries; it's a lot of changes and it's best if you understand and are comfortable with them. Take your time. I'm "officially" done with any major changes to this PR, other than any fixups we decide on, and of course re-sanitizing the history. Sorry if the ground shifted from under you during this!

@jdtsmith jdtsmith mentioned this pull request May 25, 2019
@jdtsmith
Copy link
Author

jdtsmith commented Jun 3, 2019

Any further progress on your review @immerrr? Let me know if anything needs attention.

@immerrr
Copy link
Owner

immerrr commented Jun 6, 2019

Apologies for the delay, last couple of weeks have been crazy. I'll try to have a proper look this weekend.

Copy link
Owner

@immerrr immerrr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

It is not a good sign that after having taken a week off constant reviews I had to re-learn what was happening there :) But there's indeed a lot of stuff going on, hopefully we can get this in soon.

@@ -304,7 +315,7 @@ If the latter is nil, the keymap translates into `lua-mode-map' verbatim.")
key like `{' is pressed")
(make-variable-buffer-local 'lua-electric-flag)

(defcustom lua-prompt-regexp "[^\n]*\\(>[\t ]+\\)+$"
(defcustom lua-prompt-regexp "^\\(>[\t ]+\\)+"
Copy link
Owner

Choose a reason for hiding this comment

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

My version does

> io.write("asdf")
asdf> io.write("zzz")
zzz> 

It should be fine as the first version with a TODO/FIXME, but I don't quite like that there is an infinite loop

      (while (not comint-redirect-completed)
	(accept-process-output process))

that depends on this regexp. Let's get this in and then think if there is a more reliable way to indicate the end.

lua-mode.el Show resolved Hide resolved
`(
,(concat "dofile(\"" lua-process-complete-code-filename "\")")
"os.execute('stty -echo -onlcr')"
"local loadstring = loadstring or load"
Copy link
Owner

Choose a reason for hiding this comment

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

5.3 compatibility incoming, nice 👍

Copy link
Author

Choose a reason for hiding this comment

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

I used to have a finite loop with a timed accept-process-output, but removed it. I don't think that that timeout does quite what we want, in the sense that if output continues to accrue (but no prompt has yet appeared), we want to keep waiting for it. It also is brittle to select a specific timeout period (2s, 5s, etc.); what if a given command has a long delay of some sort? What if it's a very low power system (older Raspberry PI, for example)? In the end I decided to punt on a specific choice and consider that C-g is a perfectly fine option for a hung interactive shell.

lua-mode.el Outdated Show resolved Hide resolved
lua-mode.el Outdated Show resolved Hide resolved

(defvar lua-top-level-local-regexp
"^local\\s-+\\([^ \n]+\\)\\s-*="
"A regexp to match top-level local definitions")
Copy link
Owner

Choose a reason for hiding this comment

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

I think readability here could be improved greatly with lua-rx, but let's leave it for posterity.

Copy link
Author

Choose a reason for hiding this comment

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

This was inherited from the initial PR; certainly open to contributions for readability.

lua-mode.el Show resolved Hide resolved
if type(v) == "table" then queue:push({pre = pre,entries = v}) end
end
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

Wait, I just realized, this code traversing the tables beyond the initial completion, e.g. you request
foo.b and with foo.bar being a valid completion option, it would also go ahead and complete it as foo.bar.baz, foo.bar.qux and so on, right? If my understanding is correct, at the risk of sounding rude when the implementation has already come this far, I'd say that's too much to my liking. I am accustomed to step-by-step completions, e.g. when you complete one level, input dot and complete the next one. Is there a performance reason to fetch completions in batches of 1000s?

And just out of curiosity, do you have a precedent for this kind of behaviour in other Emacs modes to see if maybe I am lagging behind the Emacs innovation frontier?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, and this took some real doing, it's the whole point of the breadth-first search. This is the strongly recommended approach to completion by the completion-at-point facility. The reason is that there are various completion "styles", and the completion backend is supposed to be agnostic about them, returning a "full list" and letting the completion style sort out what it wants. For example, initials. On that topic the completion-at-point-functions docs include:

NOTE: These functions should be cheap to run since they’re sometimes
run from ‘post-command-hook’; and they should ideally only choose
which kind of completion table to use, and not pre-filter it based
on the current text between START and END (e.g., they should not
obey ‘completion-styles’).

and in the manual:

This specifies an all-completions operation. The function should return a list of all possible completions of the specified string.

The advantage here is that you can prefetch the deep tree (which is often only dozens deep), and then it is cached for subsequent completes, and all the tests and other calls the completion framework make. It is sorted so that the "first after dot" is up front, so basically you can ignore it. It radically cuts down on the back and forth traffic to the lua shell.

Have you tried it out? Effectively it does the same thing: complete to the next dot (unless there is only ONE completion beyond that dot, which would be rare). But far more efficiently and with the option to try some crazy completion styles with company mode, etc.

Copy link
Owner

@immerrr immerrr Jun 14, 2019

Choose a reason for hiding this comment

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

It is not that I am particularly against "deep" completions, although admittedly I prefer the shorter version and no craziness.

It's just that we already do completions on a live interpreter that could produce some side-effects, we have to be careful about it. With "deep" completions it takes one object hidden somewhere within the initial 1000 element set to have a __pairs method doing something crazy to have a really bad experience with this. And I don't mean malevolent, just crazy. With "short" completions I as a user have some control over whose keys I want to be iterated in the background and whose not. With "deep" completions I am at the mercy of the code of the library, or I have to verify each library with my own eyes.

It radically cuts down on the back and forth traffic to the lua shell.

Many Pandora boxes have been opened with good intentions to optimize things. Spectre/Meltdown family of CPU vulnerabilities immediately come to mind, but surely there are more.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I hadn't thought of rogue objects overriding __pairs. Can you mock up an example? Maybe there's a way to protect against that. Deep completions are now a fundamental part of modern Emacs completion framework, for better or worse. Given a starting fragment, you are ideally to return all possible completions of that fragment without prejudice, which enables many modern forms of completion that many people use. If this is a deal-breaker for you, I really do wish you had voiced the concerns back when I first proposed a depth-first search of the globals tree, and before I spent ~days implementing it.

Copy link
Owner

Choose a reason for hiding this comment

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

If you don't mind, I'll respond to this as a generic comment, as this is a quite deep topic (no pun intended) and it becomes increasingly difficult to keep track of all the "line comment threads" as lines are updated and the comments themselves become outdated.

scripts/emacs_lua_complete.lua Show resolved Hide resolved
scripts/emacs_lua_complete.lua Outdated Show resolved Hide resolved
immerrr added a commit to immerrr/melpa that referenced this pull request Jun 11, 2019
There's an [upcoming change](immerrr/lua-mode#148) that would require to accompany the source code with a Lua initializer script for the inferior shell. This is to ensure the script is redistributed accordingly. 

Please note, that `scripts` directory is not there yet, and I'm not sure if the recipe works without it. If that's the case we can wait with merging until the scripts directory is created.
@immerrr immerrr mentioned this pull request Jun 11, 2019
7 tasks
@immerrr
Copy link
Owner

immerrr commented Jun 14, 2019

Let me have another go over the comments...

local q=queue:pop()
for k,v in pairs(q.entries) do
if type(k) == "string" and k:sub(1,1) ~= "_" then
pre = q.pre.."."..k
Copy link
Owner

Choose a reason for hiding this comment

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

Remember you told me that when keys come from completions, they are verified that they don't contain whitespaces or dots? Here, if I'm not mistaken, k comes from Lua tables and no verification is done, and thus k can contain whatever and produce completions that are invalid syntax or don't work as expected. WDYT?

Copy link
Author

@jdtsmith jdtsmith Jun 14, 2019

Choose a reason for hiding this comment

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

Good point. Yeah any valid key in Lua can be in k, including keys with whitespace. But you can't reference such keys with dot-syntax (I just tried). Since we are only parsing and completing dot-syntax I think we can get away with it. It would be better of course to parse a[" foo "].b[" c"] style things too, but that's a new can of worms. If that ever comes to pass, you'd trim and recreate whitespace around dots, but NOT within brackets (since there it is significant and must be preserved). I guess it might not be that hard to take an arbitrary expression with brackets and dots (and comments) and turn it into a series of parts with all the ignorable whitespace removed and all the relevant whitespace retained. Right now it's this simple bit:

 (mapcar 'lua-completion-trim-input (split-string expr "\\."))

Wouldn't be terrible to update this to take the expression and remove comments, find brackets etc. Right now lua-start-of-expression doesn't handle brackets gracefully.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the simplest solution is to just filter out any keys that are not alphanumeric-plus-underscore here.

@immerrr
Copy link
Owner

immerrr commented Jun 14, 2019

I have added pr-148-suggestions branch, feel free to have a look.

@immerrr
Copy link
Owner

immerrr commented Jun 15, 2019

Hmm, I hadn't thought of rogue objects overriding __pairs. Can you mock up an example? Maybe there's a way to protect against that.

I have tried searching online, the things I was able to find were not exactly rogue, but you can probably get the idea:

There's a function that I've seen on various occasions that is supposed to help avoid the __pairs metamethod.

M = {
   __pairs = function (tbl)
      return pairs({1, 2, 3})
   end
}

foo = setmetatable({foo=1, bar=2}, M)

[[
> for k, v in pairs(foo) do print(k, v) end
1	1
2	2
3	3
]]

local mypairs = function(t) return next, t end

[[
> for k, v in mypairs(foo) do print(k, v) end
bar	2
foo	1
]]

Deep completions are now a fundamental part of modern Emacs completion framework, for better or worse.

I'd like to apologize again, I should have brought up the concern that is about to follow when you first brought it up here. I don't think I was able to follow your thought as it went from the documentation to the idea that deep completions are the way to go.

You mentioned the phrase "full list" as a hint that sub-expansions (aka "deep" completions) are expected, but I don't quite see how "full list" means "descend eagerly and recursively". It sounds to me that there is a restriction that what Emacs works with should be a closed set (so, "set of natural numbers" doesn't work because it is unbounded) and then sometimes you should be able to return it in its entirety.

You have to keep in mind that Emacs uses the same completion framework for intelligent code-based completions and for example filename completions. In filename context it makes sense that they explicitly mention "a large list of possible completion alternatives", because you can have hundreds and thousands of files in one directory and completion is meant to help you find the one you need. And speaking of file completions, from my experience the general approach is not go inside directories automatically unless there is a specific and closed set of full paths to work with, e.g. all files in a git repo or in an archive.

You were mentioning initials completion style, that I think suits "shallow" one-level completions too, e.g. when you have a class with a hundred-ish methods looking like do_this_fancy_action and with initials enabled you can actually complete it as dtfa. It is easy to see how the same thing would apply to filename expansion.

In fact I can't remember having seen a completion framework that would do "deep" and "unbounded" completions. That includes various Emacs major modes, Bash completions for various commands that had custom completions implemented, and a short period of my career when I did C++ in MS Visual Studio. Do you have an example where a completion framework would eagerly descend into the completed objects?

Given a starting fragment, you are ideally to return all possible completions of that fragment without prejudice, which enables many modern forms of completion that many people use.

I completely agree with this part, but again it all ultimately depends on what do you mean by "possible completions": whether it is "keys of a single table" or "keys of a single table and keys of any nested tables".

If this is a deal-breaker for you, I really do wish you had voiced the concerns back when I first proposed a depth-first search of the globals tree, and before I spent ~days implementing it.

I am incredibly sorry about your experience, whether we find a way for this to be included like this or not. I commend your enthusiasm in tackling such a difficult issue and your devotion in seeing it through. Without a doubt, open-source community needs more people like you :)

For me it is probably the first time that I experience the scope of a contribution changing so drastically. Admittedly, my time was and is still scarce, so I could seem on and off, but in those rare moments when I was trying to wrap my head about the implications of the changes, sometimes it felt as if there was always another piece of functionality waiting around the corner.

I presume the breadth-first implementation came somewhere between this and this comments. To give you some context, in this message it sounded to me like you agreed to postpone drastic changes until after the initial implementation is merged, so I didn't expect the changes in behaviour. Again, regardless of that, I should have been clearer in communication and more attentive to the changes themselves, I'm sorry about that.

@immerrr
Copy link
Owner

immerrr commented Jun 15, 2019

With that being said, I'd propose the following plan:

  • I'll send a message to emacs-devel mailing list to ask for clarification about "deep" completion trees and how to support them better
  • I'd probably suggest to separate completion into "tiers", something like
    • :globals-only (the safest, no require is happening in the background so no side effects are expected, ideally that would also be the default)
    • :globals-and-libs (there could be side effects, but you are more in control)
    • :globals-and-libs-nested (the current implementation and the most feature-rich one)
  • In addition to that, there is an option to use the "custom pairs function" approach can counteract the risk of running into "crazy metatables" at the expense of not having the best completions for objects using those crazy metatables

Please, let me know what do you think before doing the changes.

@jdtsmith
Copy link
Author

jdtsmith commented Jun 15, 2019

There's a function that I've seen on various occasions that is supposed to help avoid the __pairs metamethod.

M = {
   __pairs = function (tbl)
      return pairs({1, 2, 3})
   end
}

foo = setmetatable({foo=1, bar=2}, M)

[[
> for k, v in pairs(foo) do print(k, v) end
1	1
2	2
3	3
]]

local mypairs = function(t) return next, t end

[[
> for k, v in mypairs(foo) do print(k, v) end
bar	2
foo	1
]]

Does a virtual rewrite of keys like this actually break completion? I.e. aren't virtual keys "as good as" real keys, for all intents and purposes?

Deep completions are now a fundamental part of modern Emacs completion framework, for better or worse.

I'd like to apologize again, I should have brought up the concern that is about to follow when you first brought it up here. I don't think I was able to follow your thought as it went from the documentation to the idea that deep completions are the way to go.

Yeah I know, and I do apologize as well. It came pretty fast as I had some free time and interest in getting a system that works. I can see how it might have seemed out of the blue.

You mentioned the phrase "full list" as a hint that sub-expansions (aka "deep" completions) are expected, but I don't quite see how "full list" means "descend eagerly and recursively". It sounds to me that there is a restriction that what Emacs works with should be a closed set (so, "set of natural numbers" doesn't work because it is unbounded) and then sometimes you should be able to return it in its entirety.

You have to keep in mind that Emacs uses the same completion framework for intelligent code-based completions and for example filename completions. In filename context it makes sense that they explicitly mention "a large list of possible completion alternatives", because you can have hundreds and thousands of files in one directory and completion is meant to help you find the one you need. And speaking of file completions, from my experience the general approach is not go inside directories automatically unless there is a specific and closed set of full paths to work with, e.g. all files in a git repo or in an archive.

Right, so it depends on whether completing a to a.b a.b.c a.b.d a.b.c.e a.b.d.f is seen as a single entity (like the string "abc"), or a nested structure with natural non-crossable boundaries. Clearly we have different points of view here. The set is closed since it is limited to a finite (and configurable) number of entities.

But even the directory as non-crossable boundary breaks down. There is precedent even for your example of nested directories. E.g. in icomplete, C-x C-f /u/l/s/e returns the completion of /usr/local/src/emacs. And see below for another example that you can try right now, without any additional configuration or package.

You were mentioning initials completion style, that I think suits "shallow" one-level completions too, e.g. when you have a class with a hundred-ish methods looking like do_this_fancy_action and with initials enabled you can actually complete it as dtfa. It is easy to see how the same thing would apply to filename expansion.

Fuzzy completion is popular in helm, company-mode, Ido, etc. Quoting a stack exchange article:

E.g. if you open an Elisp buffer using Emacs-24.4 and the latest company-mode, if you type (wi-sy company will pop up a completion menu showing window-system, window-system-for-display and with-syntax-table.

The distinction between - and . (or /) would not, IMO, be significant to a general user.

To make it concrete, here's an example that I am already using happily that would not work with shallow "one-level-at-a-time" initials style completion (again using the deep Hammerspoon hs structure):

hs.ev.ev.ra[M-Tab]

turns into:

hs.eventtap.event.rawFlagMasks

Nice, right? If hs.ev.ev.ra is not an "initialism", as described in the completion-styles docs, I don't know what is!

Here's the crux: earlier you made an argument that optimizing can lead to suboptimal outcomes; is this a case where you feel that too long of a completion list is somehow "too heavy", so that you are performing an implicit (counter-?) optimization of your own?

IMPORTANT POINT HERE:

I'm trying to understand if your main objection is to

  1. The hidden completion traffic of (up to, though usually far less than) 2000 (or whatever) nested keys, or
  2. The resulting completion interface in which a long list is presented (with the shallowest presented first)

If it's the latter, it's possible we could keep the cached completion breadth-first search backend, but optionally prune it "on demand" to keep the completion interface simpler (but still enabling fuzzy completion of the sort I gave as an example above). For example, if we presented only one level of completion at a time, would you care that in the background lua-mode had done a deep recursive (but length limited) completion search, and cached the results? As you said before, the user simply doesn't care how and where the completion data came from. They just want their completions, fast and complete.

In fact I can't remember having seen a completion framework that would do "deep" and "unbounded" completions. That includes various Emacs major modes, Bash completions for various commands that had custom completions implemented, and a short period of my career when I did C++ in MS Visual Studio. Do you have an example where a completion framework would eagerly descend into the completed objects?

All the fuzzy matching completion frameworks (many of which are in the mini-buffer) do this. Ido, icomplete, helm, etc. Granted sometimes they have interfaces to step through "one directory at a time". But behind the scenes, they explore and cache a large part of the search space.

In fact, even for filepaths, you can demonstrate this to yourself right now (courtesy comint mode). Try this in a *lua* buffer:

> file="/us/loc/sh/em[M-Tab]

I get:

> file="/usr/local/share/emacs/

How? By recursively searching the directory tree for things that match!

Given a starting fragment, you are ideally to return all possible completions of that fragment without prejudice, which enables many modern forms of completion that many people use.

I completely agree with this part, but again it all ultimately depends on what do you mean by "possible completions": whether it is "keys of a single table" or "keys of a single table and keys of any nested tables".

Indeed, that's the crux of the issue.

If this is a deal-breaker for you, I really do wish you had voiced the concerns back when I first proposed a depth-first search of the globals tree, and before I spent ~days implementing it.

I am incredibly sorry about your experience, whether we find a way for this to be included like this or not. I commend your enthusiasm in tackling such a difficult issue and your devotion in seeing it through. Without a doubt, open-source community needs more people like you :)

It's no worries, it's not my first rodeo :). I have long maintained (though now on minimal maintenance) a large Emacs major mode of over 35k lines with >25years of history (which has its own terribly powerful but terribly hackish and non-standard completion framework; hence my interest in modern Emacs-supported completion!). I know how spending large amounts of time and energy on something can make you feel about it: almost like family.

For me it is probably the first time that I experience the scope of a contribution changing so drastically. Admittedly, my time was and is still scarce, so I could seem on and off, but in those rare moments when I was trying to wrap my head about the implications of the changes, sometimes it felt as if there was always another piece of functionality waiting around the corner.

It's a strange feeling I know. I've certainly had that experience a well: a package I have known and maintained for years is suddenly attracting interest of other developers, even complete forks and ports to a new language. As anyone I think would, I felt a natural protective resistance. Eventually I sat with it, and the two things I had to ask myself were: 1) are these features and capabilities ones that I would want for the users? 2) am I willing and able to implement them myself? If I answered Yes and No, I had to consider whether my own instinctive protectiveness was serving the end goal.

I presume the breadth-first implementation came somewhere between this and this comments. To give you some context, in this message it sounded to me like you agreed to postpone drastic changes until after the initial implementation is merged, so I didn't expect the changes in behaviour. Again, regardless of that, I should have been clearer in communication and more attentive to the changes themselves, I'm sorry about that.

I admit I did move ahead without confirmation after waiting with some frustration for many days for you to digest and opine on the suggested plan, and I could have been clearer about my direction. I had intended to work on this for a ~week, and had many other pressing things I had to move on to. I could have been clearer about that. And partly it was an exercise to learn more Lua and more about modern Emacs completion (which succeeded, on both counts).

So if you want to scrap it all and wait for a 3rd person to come along and implement completion in lua-mode in another few years, I'll respect that decision. I don't have enough interest and future potential with Lua to maintain my own fork. But if that's the case, I think it's worth asking yourself why not one but two different significant efforts to contribute completion, which you have acknowledged before you yourself were unwilling to implement but very interested in having, have withered on the vine. I really don't mean to offend by this, only to offer some counsel from my own long experience. We all have our blind spots.

No rush here to think about all this. I'm going to put this down entirely until I hear from you again. I can contribute some modest effort to improving the completion interface if that's your main objection, but will not have the time to completely re-implement what in my view is a well-working and already very functional tool!

@immerrr
Copy link
Owner

immerrr commented Jun 16, 2019

If you don't mind I'll reorder the quotes so that my message makes a bit more sense when read sequentially.

It's no worries, it's not my first rodeo :). I have long maintained (though now on minimal maintenance) a large Emacs major mode of over 35k lines with >25years of history (which has its own terribly powerful but terribly hackish and non-standard completion framework; hence my interest in modern Emacs-supported completion!). I know how spending large amounts of time and energy on something can make you feel about it: almost like family.

If anything, that makes your enthusiasm even more admirable. And does not invalidate my point that more people with the attitude like yours would be welcome anywhere in the OSS community :)

It's a strange feeling I know. I've certainly had that experience a well: a package I have known and maintained for years is suddenly attracting interest of other developers, even complete forks and ports to a new language. As anyone I think would, I felt a natural protective resistance. Eventually I sat with it, and the two things I had to ask myself were: 1) are these features and capabilities ones that I would want for the users? 2) am I willing and able to implement them myself? If I answered Yes and No, I had to consider whether my own instinctive protectiveness was serving the end goal.

Ok, yes, I am a bit protective, but not so much about the code but rather about the user experience. If there was one piece of feedback I kept hearing when asking about what next big thing would help lua-mode people would answer something along the lines of "it is not broken now, it does what it is supposed to, and that's probably enough". So it kind of reinforced this approach of mine that the "continuity" comes first.

To give an example of continuity, I have the case of the unfortunate lua-mode's default indentation offset of 3 spaces. The majority of Lua code styles out there use either 2 or 4, 3 at least right now seems like a weird historical artifact that got fossilized. With respect to it, the userbase is distributed across 3 major camps:

  1. existing users who are happy with the offset of 3
  2. existing users who were unhappy with the offset of 3 and customized it
  3. new users who have the offset documented as a customization variable and can join either the first or the second camp

Now, I'm reluctant to change the default, because after doing so:

  • group 2 won't even notice when they upgrade, they have the default already overridden
  • group 3 will have a minuscule improvement: they are installing a new package, so they are reading the docs and customizing it anyway, one less "set-it-and-forget-it" step does not make much of a difference
  • group 1 however upon upgrading will see the difference which they don't like (they wanted 3 spaces after all, right?), so now they have to go and reconfigure an already configured package. I cannot tell for others, but I can get pretty annoyed when people break things while trying to fix something that IMO was not broken in the first place.

FWIW if someone came in and rewrote the entire package in a maintainable fashion while keeping the UX and provide some assurance that the users won't experience problems when upgrading I'd be ecstatic to throw out all of my code. If along comes someone who shares my POV about the continuity in the UX and is happy to share the burden of maintainership or even take over, even better.

IMPORTANT POINT HERE:

I'm trying to understand if your main objection is to

1. The _hidden completion traffic_ of (up to, though usually far less than) 2000 (or whatever) nested keys, or

2. The resulting _completion interface_ in which a long list is presented (with the shallowest presented first)

If it's the latter, it's possible we could keep the cached completion breadth-first search backend, but optionally prune it "on demand" to keep the completion interface simpler (but still enabling fuzzy completion of the sort I gave as an example above). For example, if we presented only one level of completion at a time, would you care that in the background lua-mode had done a deep recursive (but length limited) completion search, and cached the results? As you said before, the user simply doesn't care how and where the completion data came from. They just want their completions, fast and complete

With completions the continuity I'm trying to keep is more about "My workstation is safe w.r.t. side-effects that could exist in Lua code, the code is not executed unless I tell it to". And with that I try to avoid the frustration of an old-time user who decides to upgrade and then gets bitten by an absolutely random side effect. If that happened to me, I would most likely feel like the package and indirectly the maintainers would have betrayed my trust. If I were especially annoyed I would make a case of it on the Internet about how it is a potential vulnerability created out of thin air when there was no real necessity.

Naturally, the "no side-effect" invariant is impossible to maintain with completion that runs inside the inferior shell and loads libraries to be able to complete them, but I'm trying to reduce the "vector of attack", so to say, and expanding the completion tree recursively and without restrictions in my eyes does the opposite. And I think I now see how my concern can come off as protectiveness against change in general, sorry about that.

To make it concrete, here's an example that I am already using happily that would not work with shallow "one-level-at-a-time" initials style completion (again using the deep Hammerspoon hs structure):

hs.ev.ev.ra[M-Tab]

turns into:

hs.eventtap.event.rawFlagMasks

Nice, right? If hs.ev.ev.ra is not an "initialism", as described in the completion-styles docs, I don't know what is!

I think you are on to something with this example and the other ones about filenames and directories. I can confirm that file="/us/loc/sh/em expands to file="/usr/local/share/emacs/, and with this you have me convinced that there is a place for "deeper" expansions. One disagreement down, couple more to go :)

But now see how for example for file="/us/local/s[M-tab] the search tree is pruned to only 3 alternatives, /usr/local/share, /usr/local/sbin, /usr/local/src. So it looks like as if the set of candidates is restricted to all nodes at level 3 of the tree. The code in this PR (please correct me if I'm wrong) in a similar situation would continue recursive expansion and show all the keys below those 3 and all the keys below those child keys and so on. That is the behaviour that doesn't quite sound right.

Here's the crux: earlier you made an argument that optimizing can lead to suboptimal outcomes; is this a case where you feel that too long of a completion list is somehow "too heavy", so that you are performing an implicit (counter-?) optimization of your own?

Judging from the above, I don't think I have much objection against fuzzy completion or a depth-N (with any specific N > 1) search. You have helped me understand that probably my main concern is when the depth is unbounded. And that brings us to the question to the question of separators and which separators add depth, and which do not.

The distinction between - and . (or /) would not, IMO, be significant to a general user.

With this, I think I still disagree. Dashes — that come from Elisp I presume — are different from dots, because there is no namespacing in Elisp. All functions are on the same level and it makes sense to return the entire list which is, again, closed. In Lua, the closest analog to the Elisp situation with dashes and the lack of namespaces would be if Lua had all the possible completion alternatives as one big global table with variables whose names consisted of words separated by underscores. The fundamental difference between an underscore and a dot w.r.t depth is as follows:

  • for variables foo_bar_baz and fox_qux_quux, and fo as the string that you want to complete, foo or fox by themselves are not valid completion results. So it makes sense to show variable names as if they were "fully expanded". Hence there is no notion of "depth", just different number of words in variable names which can be fuzzy completed all the way.
  • for a similar table situation with two fully expanded completions being foo.bar.baz and fox.qux.quux, foo and fox can be a valid completion, so subsequent parts increase depth. IMO filename completion, as implemented right now in comint mode, does the right thing by not providing any alternatives beyond the foo/fox choice, unless you specifically ask for completing across more than 1 level of depth with smth like fo.b.b.

If we agree to do the same in Lua completions, we can reduce the depth of the object tree's traversal and with it the "attack vector" for the unexpected side effects.

Does a virtual rewrite of keys like this actually break completion? I.e. aren't virtual keys "as good as" real keys, for all intents and purposes?

Virtual rewrite of keys does not break completion, but it opens a wide field for side effects to creep in. Implementing an alternative side-effect-free pairs function is one way around this, at the cost of making the completion framework oblivious towards all that metatable magic.

I admit I did move ahead without confirmation after waiting with some frustration for many days for you to digest and opine on the suggested plan, and I could have been clearer about my direction. I had intended to work on this for a ~week, and had many other pressing things I had to move on to. I could have been clearer about that. And partly it was an exercise to learn more Lua and more about modern Emacs completion (which succeeded, on both counts).

Again, sorry about the frustration. I'm doing it on a best-effort principle, life and day job come first.

So if you want to scrap it all and wait for a 3rd person to come along and implement completion in lua-mode in another few years, I'll respect that decision. I don't have enough interest and future potential with Lua to maintain my own fork. But if that's the case, I think it's worth asking yourself why not one but two different significant efforts to contribute completion, which you have acknowledged before you yourself were unwilling to implement but very interested in having, have withered on the vine. I really don't mean to offend by this, only to offer some counsel from my own long experience. We all have our blind spots.

No rush here to think about all this. I'm going to put this down entirely until I hear from you again. I can contribute some modest effort to improving the completion interface if that's your main objection, but will not have the time to completely re-implement what in my view is a well-working and already very functional tool!

There are 2 blockers that I see right now:

  • have melpa recipe updated, so that people that install the newer version don't end up without emacs_lua_complete.lua file
  • have it disabled by default so that existing users that are used to "no-side-effects" approach are not surprised by it OR (ideally, AND) when completion is enabled, reduce the amount of expansions performed as much as possible (performance considerations come later)

Worst case scenario, if I bore you to the point where you would feel inclined to close this issue and move on with your life, I think with your blessing I'll be able to try to fix those blockers on my own. Hopefully, that would happen sooner than the 3rd person would arrive to the scene.

@jdtsmith
Copy link
Author

Sorry I never followed up on this. I do like the idea of "do depth completion only if requested", ala a deep initials-style. But of course for simplicity, you could still generate and cache the full list from the lua process, but choose only to show limit portions of it.

In any case, you have my full blessing to hack on this branch and fix the blockers you mention. Still using this branch with success, but not working with lua too much lately.

@jdtsmith jdtsmith closed this Dec 6, 2021
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.

2 participants