vim-syntastic / syntastic

Syntax checking hacks for vim
Do What The F*ck You Want To Public License
11.3k stars 1.14k forks source link

Elixir checker executes code #1141

Open nathanl opened 10 years ago

nathanl commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

nathanl commented 10 years ago

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 commented 9 years ago

Any update on this issue?

lcd047 commented 9 years ago

@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 commented 9 years ago

Gotcha, thanks!

ericlathrop commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

killerswan commented 9 years ago

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 https://github.com/elixir-lang/elixir/issues/3282) had any luck with anything yet? :)

mattly commented 9 years ago

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 commented 9 years ago

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

lcd047 commented 9 years ago

@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 commented 9 years ago

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:

jadercorrea commented 9 years ago

@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 commented 9 years ago

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 commented 6 years ago

Does this issue still exist?

lpil commented 6 years ago

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 commented 6 years ago

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

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

lpil commented 6 years ago

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 commented 6 years ago

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):

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

lpil commented 6 years ago

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 commented 6 years ago

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

skull-squadron commented 6 years ago

@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