Proposal: IO.error/2

[Dallin Osmun]

I propose that we add `IO.error/2` which matches the signature of `IO.warn/2`. It emits error diagnostics/messages instead of warnings and it causes the compile step to finish with :error.

Reasoning

Often when building a macro you'll do some validation and `raise` an error if something isn't right. This creates a poor experience for the person using the macro for two reasons:

1. You can only raise one error at a time

2. You don't get a good picture of where the error is because your whole editor turns red

You can solve both of those problems by using `IO.warn/2`. You can emit multiple warnings in a single compile step and you can pass in a stacktrace which gets turned into a Compiler Diagnostic that in turn, creates good editor hints. But now the compilation succeeds and you're left in a bad state.

I think it is useful to see multiple errors at a time because it shortens the feedback loop. It also gives more context and can help you realize where the root cause of your issue lies.

I think it would be useful to have a function that shared the properties of `IO.warn/2` and `raise/1`:

- I can emit multiple messages during a single compilation run

- These messages are output as Compiler Diagnostic errors

- Invoking this function will ultimately cause the compilation step to result in an :error

Examples

Today in Ecto Query, this snippet will cause the entire file to turn red because we mistakenly used sort_by instead of order_by.

query p in Product, sort_by: p.price

Lets assume we forgot to load the :user type in this Absinthe Schema. We have two errors but only one gets displayed. Once again, the entire editor turns red. If instead we saw each line that referenced :user turn red it might remind us more quickly that we forgot to call `import_types MyApp.UserTypes`.

query name: "Query" do
  field :user, :user, resolve: &MyApp.Users.get_user/3
  ...
  field :users, list_of(:user), resolve: &MyApp.Users.list_user/3
  ...
end

I'm currently working on a library to create GraphQL queries. I can detect three errors in the following snippet. Today I can only report one at a time and am greeted with the wall of red. If I switch to `IO.warn/2` I can report them all and see nice editor hints but my user is left with broken code after compile.

query do
  user(id: "should_be_a_number") do # wrong type for id
    firstNam # misspelled, should be firstName
    lastName do # lastName should not have a do...end block
      name
    end
  end
end

Code

Here is a patch that adds `IO.error/2`:

https://github.com/elixir-lang/elixir/compare/master...numso:io-error

[Dallin Osmun]

I failed to mention: I am using vscode with https://github.com/elixir-lsp/elixir-ls. I don't know if the experience mentioned above differs when using other editors/plugins.

[Dallin Osmun]

I understand at first glance this proposal might not seem necessary. I wanted to give some insight into how I got here. I'd like to outline a couple of the alternatives I tried and some of the issues I ran into while doing so.

As a reminder: the goal is to emit multiple error diagnostics during the compilation step and fail the compilation.

Let's stick with `raise/1`

Everyone is already using raise to emit errors from macros. The compiler diagnostic that is emitted from raise is missing a position but it does have a stacktrace. While the original frame in that trace does point to your macro, it probably still isn't the correct line. Take the following pseudocode. raise will cause a stacktrace that points to `query` on line 1 when your actual error is on line 3. Currently, raise does not and cannot know exactly where your error is.

```

1. query Thing do

2.   id

3.   error_here

4. end

```

So add `raise/2`

Maybe we enhance raise so it takes an optional stacktrace as a second argument like IO.warn does. While I think this is a great idea, it doesn't meet one of the two criteria above (emit multiple errors). For the record though, I do think there are cases out there where rather than let your macro get into an inconsistent state you would want to raise an error and stop compilation. If we allowed a custom stacktrace to be passed in to raise then the error diagnostic it emitted would be more useful.

Why not use IO.warn/2 with the `warnings_as_errors` flag?

This solution does indeed solve both my criteria: multiple errors are emitted and the build fails. But the developer experience is not ideal:

- I am forcing my users to add a compiler flag to their project. It's one more thing to remember when using my library.

- As a macro author I would like to emit both warnings and errors. If I can only emit warnings (which are treated as errors) then I am unable to distinguish between the two.

- This forces ALL warnings in your project to be treated as errors which may not be desirable in some cases.

How about IO.warn/2 and return a raise if you hit any warnings?

So let's imagine then that I use IO.warn to report all of my errors. If I had to report any errors then I'll make my macro output an AST for `raise "broken macro: check the logs"`. I don't have to force the `warnings_as_errors` flag on my users this way and I am able to emit multiple errors. But now the compilation is successful. I have to rely on my users actually exercising their code or having a good test suite to find out that the output of the macro isn't actually going to work.

On Monday, May 4, 2020 at 5:37:14 PM UTC-6, Dallin Osmun wrote:

[Ben Wilson]

Perhaps you addressed this, but how would IO.error behave when not compiling things? How would it behave if you remote console into a running system and call IO.error?

[Dallin Osmun]

Whoops, I failed to speak to that point. Thanks for asking!

If not invoked during the compilation step, I would expect IO.error to log an error and stacktrace to the console. No exceptions would be raised and your program or remote session would continue as normal. It makes sense to me that IO.warn and IO.error would be as symmetrical as possible:

IO.warn prints out a warning and a stacktrace to the console. If compiling, IO.warn also emits a warning compiler diagnostic.

IO.error prints out an error and a stacktrace to the console. If compiling, IO.error also emits an error compiler diagnostic (which is what causes the compilation to fail).

Is that what you would expect? I'm definitely open to alternatives.

[Dallin Osmun]

I just looked into how one would test code that used IO.error (by using the patch I linked to in the original post).

Currently, if your macro code throws an error like this:

raise MyError, "Something broke"

Then your test code probably looks something like:

assert_raise MyError, fn ->
  Code.require_file("fixtures/my_file.exs")
end)

With IO.error your macro code would instead have:

IO.error("Something broke")

And to test it, you would call:

assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
  Code.require_file("fixtures/my_file.exs")
end) =~ "Something broke"

While the code is indeed testable, I'm not sure if it is ideal. We may want to add another macro to ExUnit.Assertions to help test code that uses IO.warn and IO.error. Or maybe string matching in tests is good enough? Thoughts?

[José Valim]

Hi Dallin,

Thank you for the detailed proposal.

It is important to remember that in Elixir compilation and execution steps are not necessarily distinct. For example, someone could have this project:

# lib/a.ex

defmodule A do

  use UsesYourMacro

  query do

    ...

  end

end

# lib/b.ex

defmodule B do

  A.query

end

In other words, B can already try to execute the code from A before compilation finishes. Therefore, "error" can be misleading because continuing compilation with an error can lead to cascading failures, and causing the user to chase a bug that's not the original one.

At best, you would need to consider mixing "IO.error" and "raise". My suggestion in your case is to emit warnings for all errors in a particular query definition, allowing the user to get multiple reports, and then raise at the end of that query definition if any warning was emitted.

This doesn't provide warnings across all files but it can be a good starting point, especially because we indeed cannot continue beyond that file for the reasons I mentioned above.

 

[Dallin Osmun]

Hi José,

Thanks for the helpful response!

What you are saying makes sense. I must admit I don't have a very advanced knowledge of metaprogramming in elixir so I'll have to learn/practice a lot more before I can come up with an informed response.

I tried out your suggestion and wanted to report back on how it went. Using IO.warn to report problems worked well. However, two issues arose when I added this line to the end of the macro:

raise "There are errors in your query"

Here is the code in question:

defmodule Queries do

  use MyGoblet

  query "First" do

    error1

    error2

  end

  query "Second" do

    error3

    error4

  end

end

1. The first call to the macro ran, reported warnings, and then raised, halting compilation. error3 and error4 were then never reported. This can be resolved by calling raise in an @after_compile hook but I fear doing so may suffer from the same issues you described above. Unless you have any other suggestions I think that this issue, while not ideal, is still acceptable.

2. Calling raise inside the macro caused the entire file in my editor to turn red which made it next-to-impossible to see the original warnings (see the attached screenshot). This was the diagnostic that was reported from elixir:

%Mix.Task.Compiler.Diagnostic{

  compiler_name: "Elixir",

  details: nil,

  file: "/Users/dallin/code/goblet/lib/testing.ex",

  message: "** (RuntimeError) There are errors in your query\n    (goblet 0.1.0) lib/goblet.ex:74: Goblet.build/6\n    expanding macro: MyGoblet.query/2\n    lib/testing.ex:8: Queries (module)\n",

  position: nil,

  severity: :error

}

Even though the stack trace has a line number in it, that position is missing at the root of the diagnostic. This is what ultimately causes the issue. Would you like me to file this as a bug in the elixir issue tracker? Or should I instead work with the authors of the editor plugin to handle position-less Diagnostics differently?

[José Valim]

1. Yes, the granularity you get is per macro.

2. When emitting the warning, do "Macro.Env.stacktrace(__CALLER__)" to get the actual caller. You may also need to look at the AST to get the proper line. Note you would need to do the same if we were to introduce some sort of IO.error.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/ad4e81d8-6495-4511-ad7b-f6064dd1a95c%40googlegroups.com.

[Dallin Osmun]

I must be missing something. I am already using Macro.Env.stacktrace when emitting my warnings and that's working great. The problem happens when calling raise. Is there a way to pass a new stacktrace into raise? The docs for Kernel.raise say it accepts either a message or an exception and attributes. Kernel.reraise takes in a stacktrace but when I use that I only see changes in the compiler diagnostic's message; not in it's file nor position.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-l...@googlegroups.com.

[José Valim]

You can use reraise/3. It is a bit misnamed, but it is correct. But this behaviour in particular sounds like a bug in the editor itself, given it does have a line in the stacktrace specific to that file.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/704b0050-1e98-4322-b63f-34ebdc36e324%40googlegroups.com.

[cward]

It seems unreasonable for the editor to infer the line number from the `message` key. Am I wrong in thinking that? It seems that `position` is getting lost at some point in translation. Why would the `message` string contain the full stacktrace with line number, but `position` is `nil`?

After reading this thread, I now realize that this is actually somewhat common. I don't have a use case off the top of my head, but it's often that the `position` key is lost when an error comes from a macro.

In the case of elixir_ls (vscode language server), the position is expected, and the fallback is to highlight the whole file: https://github.com/elixir-lsp/elixir-ls/blob/master/apps/language_server/lib/language_server/build.ex#L63

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/704b0050-1e98-4322-b63f-34ebdc36e324%40googlegroups.com.

[José Valim]

Given Elixir compile time is really executing code, then I think it would be a nice feature to attempt to extract it from exceptions, although I understand it can be brittle.

If the error is really a compile-time reason (invalid syntax, invalid call, etc), then Elixir does show a proper error with file:line.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/d92d2a86-2b27-406f-9d70-a96600fc51ed%40googlegroups.com.

[Dallin Osmun]

Hi José

Thank you so much for taking the time to respond to this thread. It has been incredibly helpful and is very much appreciated!

I think I may have found an issue with how line numbers are determined for exceptions raised in macros. I submitted a PR here: https://github.com/elixir-lang/elixir/pull/10040

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/d92d2a86-2b27-406f-9d70-a96600fc51ed%40googlegroups.com.

[José Valim]

Awesome job on the PR!

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/92e66d66-8ab6-44e1-a69c-33a82b88d791%40googlegroups.com.

[Jason Axelson]

Hi Dallin, I'm one of the ElixirLS maintainers. In regards to VSCode showing up as all red when there's a compilation error in a macro, the behavior of ElixirLS was improved in 0.4.0 (released yesterday) to detect the line number in more cases: https://github.com/elixir-lsp/elixir-ls/pull/241

Full release announcement https://elixirforum.com/t/introducing-elixirls-the-elixir-language-server/5857/97?u=axelson

And thanks for making that PR into Elixir, that looks like it'll improve thing even further!

-Jason

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/704b0050-1e98-4322-b63f-34ebdc36e324%40googlegroups.com.