Trying to locate the proper class in a macro

Help & Support
1

Context

In Lucky, we say that all actions must return a Lucky::TextResponse, and if your action doesn’t, it’ll raise a compile-time error telling you.

The Issue

Related: https://github.com/luckyframework/lucky/issues/460#issuecomment-474616588

The error that’s raised is called from

  private def handle_response(_response : T) forall T
    {%
      raise <<-ERROR
      #{@type} returned #{T}, but it must return a Lucky::Response.
      Try this...
        ▸ Make sure to use a method like `render`, `redirect`, or `json` at the end of your action.
        ▸ If you are using a conditional, make sure all branches return a Lucky::Response.
      ERROR
    %}
  end

But in this case, @type will actually return the last class listed in your directory of Actions.

Using this image, the actual error is in Home::Index, but @type returns SignUps::New.

Screen Shot 2020-03-13 at 9.32.31 AM
Screen Shot 2020-03-13 at 9.32.31 AM750×672 64.9 KB

Is there a way to somehow mark where this error is actually coming from? Or are there other macro variables available that may give insight in to return Home::Index?

2

Any way that you can provide reduced code that reproduces the issue? I tried this:

module Foo
  def foo
    response = bar
    handle_response(response)
  end

  private def handle_response(_response : String)
    puts "OKAY!"
  end

  private def handle_response(_response : T) forall T
    {%
      raise "#{@type} returned #{T}, but it must return a String."
    %}
  end
end

abstract class Base
end

class Bar < Base
  include Foo

  def bar
    "hello"
  end
end

class Baz < Base
  include Foo

  def bar
    1
  end
end

(Bar.new || Baz.new).foo

It works fine.

3

I’m not sure if this is exactly the same or not, but here’s my go at a minimal:

https://play.crystal-lang.org/#/r/8pv9

I have a feeling there’s a lot more going on just because of how Lucky works. I can try and make a minimal lucky app if that helps

This example just shows that it seems to always be the last one (which could get hairy if you have 100 classes…)
Edit: https://play.crystal-lang.org/#/r/8pvb

4

Oh, I see what’s going on. The thing is that response = call will be evaluated on the abstract type and so the type of response is Int32 | String. The handle_response provides an overload for Int32 and another for T. The compiler doesn’t discards type from a union when an overload matches, so T here will be Int32 | String. And that will trigger for the base type and the error will be executed on all types, but only the first one (according to some ordering) will trigger the error, and so you get it on Four.

One way to solve it:

  • Add a dummy {% @type %} on run. This will make it be a macro method, meaning that you will get a different method on each type, not one in the abstract type. It’s hacky but it works.

I’m thinking of other ways but I don’t know…

A proper solution might be to have a way to mark methods as “macro methods” like we used in the past. But it’s still pretty low-level.

1 Like
5

So like this: https://play.crystal-lang.org/#/r/8pvc

6

But why not make call abstract and force a return type there? The compiler will do the work for you.

7

This comes from the macro methods

get "/whatever" do
  # must return Lucky::TextResponse here
end

I’ll have to dig in more to see if we can just let the compiler handle it. We might just have a missing type that will fix it all. I didn’t know throwing macro code in to a method makes that method a macro. I’ll play around with your suggest there and see where that takes me. Thanks!

8

We definitely can use an abstract method and I thinks that what we ended up doing, but we’d like a better error message so people know what they need to do to fix it. If they see they need to return a Lucky::Response they may not know what to do.

1 Like
9

What I’d really love to see (and have proposed earlier but didn’t push that hard on) is a way to give custom messages that the compiler can use. But that’s probably whole different discussion.

I’d love something like

@[CrystalReturnError("message to give when return type doesn't match")]
def render : Lucky::Response
end

I have no idea how or if that would work, but something like that would be incredible

2 Likes
10

That only happens when you mention @type inside a macro. The rationale behind that is that the method needs to be specialized depending on the @type so we make a separate instance for it, never reusing it if it’s, say, an abstract type and the method is not overridden in subclasses.

That’s why I suggested adding a dummy {% @type %}… but I suggest a good comment like “this makes Crystal generate this method separately for each different type”.

3 Likes
11

Thanks for your help @asterite!! I hope this works