macros: Keep the location (file/line/column) of AST nodes

[bew]

To get proper location in case of bad usage of macros we can use location pragma around a specific AST node, but it's very repetitive to do it each time, and so nobody does that (and also it's not really documented either).

Here is an example:

macro do_with_location(node)
  do_something(
    #<loc:{{ node.filename }},{{ node.line_number }},{{ node.column_number }}>{{ node }}
  )
end

do_with_location BadConst

This gives the proper error on BadConst:

Error in loc.cr:8: expanding macro

do_with_location BadConst
^~~~~~~~~~~~~~~~

in loc.cr:8: undefined constant BadConst

do_with_location BadConst
                 ^~~~~~~~

I think it's a good candidate for a macro method that does that when writing:

macro do_with_location(node)
  do_something {{ node.with_location }}
end

For stdlib usage I think this could make JSON.mapping error more clean when a type doesn't exist (instead of the huuuuge error https://carc.in/#/r/44p0 for example)

WDYT?

[bew]

I did a simple implementation:

diff --git a/src/compiler/crystal/macros/methods.cr b/src/compiler/crystal/macros/methods.cr
index 724d27aaf..1b3ee42fd 100644
--- a/src/compiler/crystal/macros/methods.cr
+++ b/src/compiler/crystal/macros/methods.cr
@@ -319,6 +319,12 @@ module Crystal
           column_number = end_location.try &.original_location.try &.column_number
           column_number ? NumberLiteral.new(column_number) : NilLiteral.new
         end
+      when "with_location"
+        interpret_argless_method("with_location", args) do
+          MacroId.new(String.build do |str|
+            to_s(str, emit_loc_pragma: true)
+          end)
+        end
       when "=="
         interpret_one_arg_method(method, args) do |arg|
           BoolLiteral.new(self == arg)

It seems to work well for the tests I did.
The only "bad" thing is when a macro calls another macro and passes a node with location, the error message is cluttered with the location info, but that can probably be changed in the formatting of the error message.

I can start a WIP PR with this diff if you want

[asterite]

You explained what you want to do, but not why.

[asterite]

Oh, I see the reason now.

I don't think pushing the responsibility of showing nice error messages to the user is a good idea. We should think of how to improve that long error message. Maybe not show the whole macro source, and only show it when --trace is passed?

[bew]

I agree the macro source is a bit too much, 'hiding' it under --error-trace seems a good idea!

But then what do you want to show the user when --error-trace is not activated?

If we want the nice error message as I showed, without the user telling us where the token is coming from, I fear this is impossible: The macro language as a 'templating'-like language makes token tracing impossible, or I have no idea how..
Please correct me if I'm wrong!

[bew]

Any other opinions?

[paulcsmith]

I opened a couple issues around this because macros made it hard to tell what was happening. They were closed I think because I didn't explain myself clearly and because of a misunderstanding in how macros were expanded: #4700 and #4699

Here's my attempt to clarify:

• I think it would make sense to show the macro source, but only a few lines around it, not necessarily the whole thing as sometimes they are pretty big and complex. Elixir by default doesn't show the macro source which makes for a clean stack trace but it is often hard to see why your code didn't work because you don't know what the macro was doing. Having at least a few lines of macro code visible by default would be great. Running with --error-trace would show the whole macro as suggested.
• Show user code at the bottom (the code that called the macro). Even though it is technically correct for the expanded macro to show up at the bottom, from a user's perspective they'd rather see the code they wrote at the bottom and is the more intuitive thing. Everyone I've worked with has thought the same so I think this is a common issue that can be resolved by putting the caller at the bottom.

Here is what I imagine the code you wrote would look like with what I'm proposing:

https://gist.github.com/paulcsmith/52bad9e5862c92bf535edeee97dfbc66

Error in line 4: expanding macro

in line 4: expanding macro

# Brief snippet of the expanded macro
expanding macro
in macro 'mapping' /usr/lib/crystal/json/mapping.cr:63, line 6:

# Tell user how to see the whole macro if that helps them
Compile with --error-trace to see the fully expanded macro

   5.     
>  6.       @var : BadType 
   7.    
   9.         def var=(_var : BadType )
  10.           @var = _var
  11.         end
 
  
# Move this to the bottom since it is the thing the user typed. It's what I always want to see first
expanding macro
in macro 'mapping' /usr/lib/crystal/json/mapping.cr:219, line 1:

>  1.     ::JSON.mapping({var: BadType})
   2.   

undefined constant BadType

I don't know how difficult this would be, but I think it'd be a huge step in the right direction. Even just making macro code condensed would make it much easier to navigate compile errors

[bew]

A good example of the with_location feature is https://groups.google.com/forum/#!topic/crystal-lang/kwL9tuDKJgw where you have a DSL as following:

macro create_class(name, file = __FILE__, line = __LINE__, &block)
  class {{name.id}}
    {{block.body}}
  end
end

macro create_method(name, file = __FILE__, line = __LINE__, &block)
  def {{name.id}}
    puts "{{file.id}}:{{line.id}}"
    {{block.body}}
  end
end

create_class Hello do
  # Blank lines
  # to help
  # demonstrate
  # this example.
  create_method say do
    "Hello"
  end

  create_method yell do
    "HELLO"
  end
end

Hello.new.say # test.cr:14
Hello.new.yell # test.cr:14

We see at the end that the reported lines in create_method macro are incorrect, and they all point to the create_class macro call (line 14).

With ASTNode#with_location, the above code can be rewritten as:

macro create_class(name, &block)
  class {{name.with_location.id}}
    {{block.body.with_location}}
  end
end

macro create_method(name, file = __FILE__, line = __LINE__, &block)
  def {{name.with_location.id}}
    puts "{{file.id}}:{{line.id}}"
    {{block.body.with_location}}
  end
end

create_class Hello do
  # Blank lines
  # to help
  # demonstrate
  # this example.
  create_method say do
    "Hello"
  end

  create_method yell do
    "HELLO"
  end
end

Hello.new.say # test.cr:19
Hello.new.yell # test.cr:23

And the create_method macros will report the lines where they are actually called (lines 19 & 23)

[bew]

Note: here is another example, that doesn't work yet even when adding some with_location (might need a change in how macro's raise is reported):

macro raise_on(node)
  {% node.raise "node '#{node}' is here!" %}
end

macro will_raise_soon(node)
  # this is a helper macro
  raise_on {{node}}
end

macro will_raise(node)
  # this is a helper macro
  will_raise_soon {{node}}
end

will_raise :this

I think this should report the error node ':this' is here! at the last line (will_raise :this), without any trace (but allow to show it with --error-trace)

[bew]

I don't think pushing the responsibility of showing nice error messages to the user is a good idea.

@asterite does that mean that you think that the user should not have to put .with_location everywhere to forward the actual location of ast nodes?

If yes, I've been thinking about it, and I think we could try to ALWAYS keep the ast nodes locations (unless the node doesn't come from the outside, e.g it could have been created inside the macro).

[bew]

Thanks @makenowjust (and the approvers!!) 😃

I'll extract the macro raise example as it's a separate issue, than I think I can close this issue

[bew]

I currently can't compile master (on arch, LLVM 7, etc..), so I can't verify that #6125 (comment) works now (the top example), can someone do this check? Thx!

[RX14]

@bew install llvm6 and you can compile the compiler fine. It won't affect anything else because the llvm 7 shared libraries remain installed.