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

emacs plugin development track #275

Open
6 of 11 tasks
wgrr opened this issue May 7, 2021 · 7 comments · Fixed by #353
Open
6 of 11 tasks

emacs plugin development track #275

wgrr opened this issue May 7, 2021 · 7 comments · Fixed by #353

Comments

@wgrr
Copy link
Contributor

wgrr commented May 7, 2021

This is a sentinel issue to track what arrived from the discussion in pull request #264

Flycheck

MELPA

according to the project's contributing, we are missing:

  • Coding style and conventions
  • Package metadata
  • Tag commits to release (optional)
  • Create a recipe file

Flymake

  • Flymake backend

LSP

lsp-mode integrates with flycheck, it should be already working, but it needs test and automated testing

  • Eglot
  • lsp-mode

NPM

Either detect quick-lint-js version currently installed with npm, or provide conveniences for users doing that, for example, ivy exposes some utility functions for filling its selection buffer from shell commands (https://oremacs.com/swiper/#global-key-bindings), we can do something akin allowing the user to choose which quick-lint-js finder function to run.
see #409 (comment)

Lint Server

currently linters just re-executes quick-lint-js to get new errors, we need a more efficient way of offering errors, our target Emacs version for that is 24.5 (ubuntu 18.04).

Emacs 25 has added dynamic modules, im not sure how well supported is Windows, I assume it has problems because some of
their api have posix specifics exposed, such as struct timespec.

So we need to decice between a C ABI, or go with a custom protocol on top of either tcp/udp/unix, rtags achives this with unix socket communication, which also needs workarounds on Windows.

@strager
Copy link
Collaborator

strager commented May 7, 2021

Add :errors-explainer blocker: #???

#206

We can now link to e.g. https://quick-lint-js.com/errors/#E001. Is an HTTP link the preferred error-explainer? Or should we bundle something with qljs or with the Emacs plugin?

@strager
Copy link
Collaborator

strager commented May 7, 2021

Emacs 25 has added dynamic modules

This looks cool! I'm concerned about crashes, hangs, and leaks in qljs causing Emacs to crash, hang, or leak, though.

A separate server is a safer (though less efficient and probably harder to implement) option.

@strager
Copy link
Collaborator

strager commented May 7, 2021

according to the project's contributing, we are missing:

  • Dedicated SCM repository

Are you referring to the following text?

Dedicated SCM repository

Keep each package in its own SCM (e.g., git) repository. This makes it possible to have a different version number for each package as MELPA stable looks at the SCM’s tags to assign version numbers to recipes.

My interpretation is that the package can't be in the same SCM repo as another package. I don't interpret this to mean that a package needs a dedicated SCM repo containing no other code.

@wgrr
Copy link
Contributor Author

wgrr commented May 8, 2021

Are you referring to the following text?

Yes, i assumed that we would have multiple packages there, but actual thinking about this it's far better to have one, all included package. Removed.

Is an HTTP link the preferred error-explainer?

ESLint checker does offer it as https://eslint.org/docs/rules/N
the error-explainer is function which takes the error and should return a message, a message is a string, url (lisp cons pair) or a lambda that takes a 'buffer' we should write error documentation to, maybe we want to define some costumization for this and default to URL, since it's the most used by error-explainers.

wgrr added a commit to wgrr/quick-lint-js that referenced this issue May 12, 2021
Fixes quick-lint#263
Updates quick-lint#275

Signed-off-by: wagner riffel <w@104d.net>
wgrr added a commit to wgrr/quick-lint-js that referenced this issue May 12, 2021
Fixes quick-lint#263
Updates quick-lint#275

Signed-off-by: wagner riffel <w@104d.net>
wgrr added a commit to wgrr/quick-lint-js that referenced this issue May 12, 2021
Fixes quick-lint#263
Updates quick-lint#275

Signed-off-by: wagner riffel <w@104d.net>
wgrr added a commit to wgrr/quick-lint-js that referenced this issue May 13, 2021
Fixes quick-lint#263
Updates quick-lint#275

Signed-off-by: wagner riffel <w@104d.net>
strager pushed a commit that referenced this issue May 14, 2021
@strager
Copy link
Collaborator

strager commented May 23, 2021

So we need to decice between a C ABI

I made src/quick-lint-js/c-api.h which exposes functions for the VS Code plugin and for the website demo. We could do something similar for the Emacs plugin. However, I'm wary of putting quick-lint-js in the same process as Emacs.

or go with a custom protocol on top of either tcp/udp/unix

Do you plan on developing this yourself? Do you want help?

@wgrr
Copy link
Contributor Author

wgrr commented May 25, 2021

Hi, sorry for the delay, somehow I missed the notification for your
update here

I thought we already had some infrastructure in the codebase for doing
sockets, I wrongly inferred that from the server part of LSP, I think it
makes sense to use process pipes just like --lsp, we can go with c-api.h
route if wish, crashing the editor would be a disaster and the only idea
I can think of is every time right before we jump in quick-lint-js code we
save our machine state and rely on signals to recover to a previous working
state. Sounds hard to get right, very machine and OS specific and an overkill
to me, pipes are possibly slower and requires a protocol but safer and easier
to port outside linux, what we choose here? I feel pipes will good enough.

The primary motivation for that is to avoid recreating quick-lint-js
process in the Flycheck and Flymake backends, I do plan to develop this,
I'm working on getting Flymake and others working first, then if pipe route
is taken I need help to decide on the protocol, for compatibility reasons
I'm leaning towards generating lisp code to run or be decoded, I feel JSON
would be nicer since we could use simdjson that we already depend on,
but a builtin JSON decoder in Emacs is available only in latest version
(27.1), we would need to maintain our own lisp decoder for a while until
Emacs 27.1 is mainstream, or bring in an external decoder.

@strager
Copy link
Collaborator

strager commented May 25, 2021

I think it
makes sense to use process pipes just like --lsp, we can go with c-api.h
route if wish, crashing the editor would be a disaster
[...], pipes are possibly slower [than in-process] and requires a protocol but safer and easier
to port outside linux, what we choose here? I feel pipes will good enough.

Does emacs support shared memory? Could we share the byte buffer used to store the source code? Or share a copy of it? If so, we could do shared memory + pipes/semaphores/something, which would give us good performance (I hope) and crash tolerance.

How fast would copying the whole buffer in emacs on each buffer change be? quick-lint-js' LSP plugin already copies the whole buffer every change (

padded_string& old_content =
this->content_buffers_[this->active_content_buffer_];
padded_string& new_content =
this->content_buffers_[1 - this->active_content_buffer_];
const char8* start = this->locator_.from_position(range.start);
const char8* end = this->locator_.from_position(range.end);
new_content.resize(narrow_cast<int>(
(start - old_content.begin()) +
narrow_cast<int>(replacement_text.size()) + (old_content.end() - end)));
char8* out = new_content.data();
out = std::copy(old_content.cbegin(), start, out);
out = std::copy(replacement_text.begin(), replacement_text.end(), out);
out = std::copy(end, old_content.cend(), out);
QLJS_ASSERT(out == new_content.end());
this->locator_.replace_text(range, replacement_text, &new_content);
this->active_content_buffer_ = 1 - this->active_content_buffer_;
), so this work is necessary no matter what approach we take. (quick-lint-js' copying is a few memcpys, so it's relatively fast, but not free.) quick-lint-js's parser needs a contiguous buffer.

Maybe I should prototype a shared memory solution myself to test this idea. This mechanism might be useful for plugins for other editors too.

I'm working on getting Flymake and others working first

👍

then if pipe route
is taken I need help to decide on the protocol, for compatibility reasons
I'm leaning towards generating lisp code to run or be decoded, I feel JSON
would be nicer since we could use simdjson that we already depend on,
but a builtin JSON decoder in Emacs is available only in latest version
(27.1), we would need to maintain our own lisp decoder for a while until
Emacs 27.1 is mainstream, or bring in an external decoder.

For quick-lint-js -> emacs messages, we can easily have quick-lint-js emit elisp. We don't use simdjson for writing JSON; simdjson only parses JSON. Writing JSON is done with string concatenation:

void lsp_error_formatter::write_before_message(severity sev,
const source_code_span &origin) {
char8 severity_type{};
switch (sev) {
case severity::error:
severity_type = u8'1';
break;
case severity::note:
// Don't write notes. Only write the main message.
return;
case severity::warning:
severity_type = u8'2';
break;
}
lsp_range r = this->locator_.range(origin);
this->output_.append_copy(u8"{\"range\":{\"start\":");
this->output_.append_copy(u8"{\"line\":");
this->output_.append_decimal_integer(r.start.line);
this->output_.append_copy(u8",\"character\":");
this->output_.append_decimal_integer(r.start.character);
this->output_.append_copy(u8"},\"end\":");
this->output_.append_copy(u8"{\"line\":");
this->output_.append_decimal_integer(r.end.line);
this->output_.append_copy(u8",\"character\":");
this->output_.append_decimal_integer(r.end.character);
this->output_.append_copy(u8"}},\"severity\":");
this->output_.append_copy(severity_type);
this->output_.append_copy(u8",\"code\":\"");
this->output_.append_copy(reinterpret_cast<const char8 *>(this->code_));
this->output_.append_copy(u8"\",\"source\":\"quick-lint-js\"");
this->output_.append_copy(u8",\"message\":\"");
}
void lsp_error_formatter::write_message_part(severity sev,
string8_view message) {
if (sev == severity::note) {
// Don't write notes. Only write the main message.
return;
}
write_json_escaped_string(this->output_, message);
}
void lsp_error_formatter::write_after_message(severity sev,
const source_code_span &) {
if (sev == severity::note) {
// Don't write notes. Only write the main message.
return;
}
this->output_.append_copy(u8"\"}");
}
I envision us doing the same for elisp.

For emacs -> quick-lint-js messages, we could do anything. I'd be fine with a custom text or binary format, or a JSON format, or an elisp format. I'm biased toward writing as little elisp as possible because I assume elisp is slow compared to C++.

@strager strager linked a pull request Jun 11, 2021 that will close this issue
@strager strager reopened this Jun 13, 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 a pull request may close this issue.

2 participants