More accurate macro errors

[Blacksmoke16]

• Tweak how macro errors are treated to retain original location
• Add test coverage for these cases

Resolves #7147

Top level raise in macro

macro macro_raise
  {% raise "OH NO" %}
end

macro_raise

Before:

$ crystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise
     ^----------
Error: OH NO

After:

$ ccrystal run --error-trace test.cr 
In test.cr:2:6

 2 | {% raise "OH NO" %}
        ^----
Error: OH NO

In test.cr:5:1

 5 | macro_raise
     ^----------
Error: OH NO

Node raise in macro

macro macro_raise_on(arg)
  {% arg.raise "OH NO" %}
end

macro_raise_on 123

Before:

$ crystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise_on 123
     ^-------------
Error: OH NO

After:

$ ccrystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise_on 123
     ^-------------
Error: OH NO

In test.cr:5:16

 5 | macro_raise_on 123
                    ^
Error: OH NO

This one is a bit weird, but I didn't think it was worth trying to figure out how to remove the first frame in only this single context.

Node raise in macro missing location

macro macro_raise_on(arg)
  {% arg.raise "foo" %}
end

macro_raise_on :this

Before:

$ crystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise_on :this
     ^-------------
Error: foo

After:

$ ccrystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise_on :this
     ^-------------
Error: foo

Node raise in complex/nested macro call

class Child
  def self.value : Nil
  end
end

module ExampleModule
  macro calculate_value
    {% begin %}
      {%
        if method = Child.class.methods.find &.name.stringify.==("value")
          method.raise "Value method must be an instance method."
        else
          raise "BUG: Didn't find value method."
        end
      %}
    {% end %}
  end

  class_getter value : Nil do
    calculate_value
  end
end

ExampleModule.value

Before:

$ crystal run --error-trace test.cr 
In test.cr:24:15

 24 | ExampleModule.value
                    ^----
Error: Value method must be an instance method.

After:

$ ccrystal run --error-trace test.cr 
In test.cr:24:15

 24 | ExampleModule.value
                    ^----
Error: Value method must be an instance method.

In test.cr:20:5

 20 | calculate_value
      ^--------------
Error: Value method must be an instance method.

In test.cr:2:12

 2 | def self.value : Nil
              ^----
Error: Value method must be an instance method.

Top level raise in method

def foo(x)
  {% raise "OH NO" %}
end

foo 1

Before:

$ crystal run --error-trace test.cr 
In test.cr:5:1

 5 | foo 1
     ^--
Error: OH NO

After:

$ ccrystal run --error-trace test.cr 
In test.cr:2:6

 2 | {% raise "OH NO" %}
        ^----
Error: OH NO

In test.cr:5:1

 5 | foo 1
     ^--
Error: OH NO

[Blacksmoke16]

I think manually creating the type exceptions via .for_node changed something that caused these calls to be taken into consideration and error due to the first arg expecting a string not the exception instance.

[Blacksmoke16]

Allows differentiating a node specific versus top level raise to make the logic simpler as each case is slightly different in how it should be ordered.

[straight-shoota]

Looks good. I'm only a bit unsure about the representation of TopLevelMacroRaiseException. Not entirely sure why. Maybe because I think {% foo.raise "" %} and {% raise "" %} should produce the same concept of an exception? I suppose an alternative implementation would be to have a property in MacroRaiseException to signal whether the raise call has a target or not.
I'm definitely fine with taking it as is, just leaving this as food for thought.

[straight-shoota]

Why not check for the concrete line number ("error in line 2"?) here like above?

[Blacksmoke16]

In this particular case, the trace looks like:

$ ./bin/crystal run --error-trace test.cr 
In test.cr:5:1

 5 | macro_raise_on 123
     ^-------------
Error: OH NO


In test.cr:5:16

 5 | macro_raise_on 123
                    ^
Error: OH NO

From the OP:

This one is a bit weird, but I didn't think it was worth trying to figure out how to remove the first frame in only this single context.

So the spec is basically asserting there are 2 frames on the same line, unlike the previous example which had two frames on two different lines.

[asterite]

Note that the frame for raise should never be shown. I implemented it that way so that a method call could produce a error just like the compiler can produce an error for it (for example, wrong number of arguments). Pointing to raise makes the error a bit more obscure, and kind of loses the point I had in mind.

The other changes look good, though!

[straight-shoota]

Yeah, that's an important point. I think this change in its entirety is still a good move forward. No existing specs are broken, so the error attribution scope was never specified in code. And as we can see from the examples in #7147 there were quite some inconsistencies.
Now we have a more clearly defined base behaviour. We can use that to adjust the error reporting for better usability in future changes.

Showing the frame of the raise call probably won't hurt anyone in the mean time.
Do you agree that we can leave this change in for 1.9 (it really is quite an improvement) and do some fine tuning afterwards?

[asterite]

Of course! I was just mentioning the intention behind not showing raise in the frame, but the overall change is better than that lost feature. And that feature isn't that important either.