Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Elixir checker executes code #1141

Open
nathanl opened this issue Jul 16, 2014 · 29 comments
Open

Elixir checker executes code #1141

nathanl opened this issue Jul 16, 2014 · 29 comments
Labels

Comments

@nathanl
Copy link

nathanl commented Jul 16, 2014

The Elixir syntax checker executes any program it checks. For example:

# sleeper.exs
sleeper = fn -> :timer.sleep(4_000) end
sleeper.()

It takes 4 seconds to save this file because the sleeper function is executed.

This method of checking is dangerous; for example, suppose it were a script to delete files? Also it's annoying; if you accidentally create infinite recursion, Vim will hang.

Can we check syntax without executing the code?

(The answer may be "only in a limited way": see this thread)

@lcd047
Copy link
Collaborator

lcd047 commented Jul 16, 2014

I'm afraid I barely know what Elixir is, let alone how it works. :) You guys will have to sort it out, and let me know what is the preferred incantation to make it work, and if it's safe to use. In the mean time, I can only offer the same treatment I applied to perl: #1013. Sorry about that.

@lcd047
Copy link
Collaborator

lcd047 commented Jul 16, 2014

Done in 1d19dff. Set g:syntastic_enable_elixir_checker to 1 in your vimrc to re-enable the checker.

@nathanl
Copy link
Author

nathanl commented Jul 16, 2014

No problem. :) I'm quite new to it myself and was just trying to figure out why it killed my editor.

Maybe when I know more, I can offer an alternate approach.

@alxndr
Copy link

alxndr commented Apr 8, 2015

Any update on this issue?

@lcd047
Copy link
Collaborator

lcd047 commented Apr 8, 2015

@alxndr It's an Elixir issue, not a syntastic one. Or, put another way: if anybody has figured out yet how to make Elixir run syntactic checks without also executing the code being checked, they didn't bother to tell me about it. Thus, no progress so far.

@alxndr
Copy link

alxndr commented Apr 8, 2015

Gotcha, thanks!

@ericlathrop
Copy link

I'm not much of an elixir expert, but can't we just change the makeprg to elixirc, which compiles the code instead of running it? I tried it, and it seems to work for me.

@lcd047
Copy link
Collaborator

lcd047 commented May 10, 2015

can't we just change the makeprg to elixirc , which compiles the code instead of running it?

Actually, elixirc is just a script around elixir. It doesn't solve any security problem, and it adds a problem of its own. But you can still use it without code changes if you insist (cf. #1343).

@ericlathrop
Copy link

Ah, I found this security issue raised on the Elixir project where José himself describes how to approach syntax error detection. Looks like Elixir can easily parse its own code, so it should be possible to have the syntastic checker bundle in an Elixir script to safely do this. Here's a first attempt at such a script, which prints syntax errors but doesn't properly handle I/O errors:

[path | _] = System.argv()
{:ok, file} = File.open(path)
data = IO.read(file, :all)
code = Code.string_to_quoted data
case code do
  {:ok, _} -> :ok
  {:error, {line, error, token}} -> IO.puts "#{path}:#{line}:#{error}#{token}"
end

I tested it on "hello world" and it doesn't execute the code.

Would it be weird to bundle a script like this in syntastic? If not, can someone point me to an example of where the script should go and how I set makeprg to the correct path?

@lcd047
Copy link
Collaborator

lcd047 commented May 11, 2015

Would it be weird to bundle a script like this in syntastic?

Not at all, other checkers already do that, f.i. erlang/escript and python/python. Still, I'd prefer the script to do error checking and exit 0 if everything went fine, or 1 if it run into abnormal conditions (I/O errors, exceptions, whatever).

Reading this post which tries to do the same thing for flycheck, the result would have many limitations compared to the existing checker, so perhaps there should be an option to switch between the "safe" and the "useful" approaches. Than again, the existing checker has its own problems (cf. #1343), and those aren't going to be solved any time soon.

If not, can someone point me to an example of where the script should go and how I set makeprg to the correct path?

The erlang/escript is a good exemple. But if you write the script I can take care of the details.

@lcd047
Copy link
Collaborator

lcd047 commented May 11, 2015

@ericlathrop: One more thing: the existing checker also uses mix. Can you please explain how would this come into play, keeping in mind that to me "elixir" is stuff that belongs in some alchemist's olde bottle?

@ericlathrop
Copy link

@lcd047 Okay, I'll work on adding error checking and pushing this further along. I just started a new job so it may take a week or two. I think mix is a project build tool, but I haven't used it because I'm just learning elixir. I'm only on Chapter 6 of the book, so I haven't gotten to mix yet.

@blacksails
Copy link

For me the current elixir syntax check is a problem because it compiles the files. The phoenix framework has a nice feature of doing a code reloading on runtime. On page requests it checks if there are files that need to be compiled, if there is it compiles them and loads the code before it serves the request. But when i save the file with vim the syntax check compiles the file, which causes phoenix not to pick it up.

@lpil
Copy link

lpil commented Jun 21, 2015

@lcd047 mix is the Elixir build tool and task runner, it is part of the standard library/distribution.

@killerswan
Copy link

For those new to the thread, a summary:

  1. To enable the built-in Elixir checker you need both let g:syntastic_elixir_checkers = ['elixir'] and let g:syntastic_enable_elixir_checker = 1.
  2. The existing elixir checker here in syntastic goes looking for a mix.exs file (indicating a mix project) and then either runs elixir __.ex or mix compile mix.exs.
  3. If you set let g:syntastic_elixir_elixir_args = '+elixirc' you'll get behavior equivalent to elixirc __.ex (and mix compile +elixirc mix.exs won't complain).
  4. All these commands execute some code, not just "compile" it.
  5. Several write lots of files locally.
  6. Furthermore, the error formats are subject to change and not parsed ideally yet.

It sounds to me like leaving it disabled by default is still the right thing. :(

And we could use a tool which was safe to run and provided an easier-to-parse output. Have @ericlathrop (how's the new job?) or @mattly (who filed elixir-lang/elixir#3282) had any luck with anything yet? :)

@mattly
Copy link

mattly commented Jul 21, 2015

I've had a lot going on in my life lately, and have been doing a lot of learning and working with Clojure for $dayjob, and haven't had time to pursue this beyond this example I posted to flycheck/flycheck#630 :

elixir -e 'r = System.argv |> List.first |> File.read! |> Code.string_to_quoted; if elem(r, 0) == :error do IO.inspect(elem(r,1)); end' -- filename.ex

@lpil
Copy link

lpil commented Jul 21, 2015

That's similar to what I'm doing in my linter project, and it works quite nicely.

@lcd047
Copy link
Collaborator

lcd047 commented Jul 21, 2015

@lpil I presume you're referring to dogma. Then perhaps the solution is to add a checker for dogma, and leave the elixir checker as it is?

@lpil
Copy link

lpil commented Jul 21, 2015

That's the one :)

While I intend to make a syntastic compatible formatter, I think we still need a plain Elixir checker for the following reasons:

  • Dogma is still in alpha. I'm likely going to break the API repeatedly before v1.0.0, and there are several problems that I don't know how to solve yet.
  • Dogma lints style as well as correctness, and is probably far too opinionated to be used as the default syntactic Elixir linter. Think more JSCS than jshint.

@jadercorrea
Copy link

@nathanl .() is a syntax for executing functions inside IEX and it will run when called or when you try to compile a script (.exs). Try to play with it as a module instead. Just tested syntastic (on a mix project) here with a very long calculation and just syntax was checked. Really cool.

But keep in mind that, since complier is not running, you may have warnings on variables (especially when using patter matching), that will broke your code when fixed.

Couldn't find any problems using syntastic so far.

@lpil
Copy link

lpil commented Sep 10, 2015

That's not correct, it is the syntax for calling an anonymous function. The syntax in iex is the same as anywhere else.

What problems with pattern matching have you encountered? There should be no difference.

@ChrisZou
Copy link

Does this issue still exist?

@lpil
Copy link

lpil commented Dec 30, 2017

Yes. Syntastic currently compiles (and thus executes) Elixir code in order to check the syntax.

A safer and more performant solution would be to check the syntax with the Elixir parser, which is distributed with the language.

@skull-squadron
Copy link

Update Dogma is deprecated and syntax checking is now built-in:

mix format --dry-run {{filename}}

@lpil
Copy link

lpil commented Aug 31, 2018

Hiya. As the creator of dogma I'd recommend switching to what @steakknife suggests, dogma is deprecated and the current approach to checking syntax is unsafe.

@lcd047
Copy link
Collaborator

lcd047 commented Aug 31, 2018

Great. Now, in order for this to actually happen, one of you guys can either post a PR, or explain for the non-initiated (i.e. for yours truly):

  • how syntastic can check that the installed elixir version supports the new options
  • how checking the current file is supposed to happen, and
  • what parts of the existing baggage can be discarded.

Posting some test files producing representative error messages would be nice, too.

@lpil
Copy link

lpil commented Aug 31, 2018

Formatting is considered correct or incorrect on an entire file basis, so no error messages will be given. It's similar to gofmt, rust-format, etc in this respect.

This was introduced in Elixir 1.6, though rather than booting the VM to check this I would instead run the command above and check for whether it wasn't recognised by checking the exit status and the content of stderr. This will be faster as you don't need to boot the VM twice.

Given the entire ecosystem is now using the formatter I'd probably not worry about older versions that do not have it.

@lcd047
Copy link
Collaborator

lcd047 commented Aug 31, 2018

Well, I suppose this leaves the other option. Working PRs are welcome.

@skull-squadron
Copy link

skull-squadron commented Aug 31, 2018

@lpil 1.6+ looks right. Here's a script based on suggestions. Verified that error output is always on stderr.

#!/usr/bin/env elixir
if Version.match?(System.version(), ">=1.6.0") do
  Mix.State.start_link(nil)
  Mix.ProjectStack.start_link(nil)
  Mix.Tasks.Format.run(System.argv() ++ ["--dry-run"])
else
  :erlang.error("Upgrade elixir to 1.6+ to enable vim-syntastic syntax checking")
end

TGIF and awesome Labor Day weekend depending

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests