Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error using generic with DB::Serializable and @[DB::Field(converter: MyConverter(T))] #189

Open
jgaskins opened this issue Sep 11, 2023 · 6 comments

Comments

@jgaskins
Copy link
Contributor

jgaskins commented Sep 11, 2023

I've got a DB model with some metadata and an arbitrary JSON payload (event data from webhooks) that looks something like this:

struct Event(T) < Model
  getter id : String
  getter type : String
  @[DB::Field(converter: Event::Converter(T))]
  getter raw : T
  getter created_at : Time

  module Converter(T)
    extend self
    def from_rs(rs : DB::ResultSet)
      T.new rs.read(JSON::PullParser)
    end
  end
end

I'm getting this error at compile time:

 6 | @[DB::Field(converter: Event::Converter(T))]
                                             ^
Error: undefined constant T

I don't know what's causing it. This seems like it should work and the macro seems to be generating the correct code:

 >  86 |               when "raw"
 >  87 |                 __temp_4290 = true
 >  88 |                 begin
 >  89 |                   __temp_4289 =
 >  90 |
 >  91 |                       Event::Converter(T).from_rs(rs)
 >  92 |
 >  93 |                 rescue exc
 >  94 |                   ::raise ::DB::MappingException.new(exc.message, self.class.to_s, "raw", cause: exc)
 >  95 |                 end

That's the only place the value[:converter] is used in DB::Serializer.

@jgaskins jgaskins changed the title Error using generic with DB::Serializable and @[DB::Field(converter: T)] Error using generic with DB::Serializable and @[DB::Field(converter: MyConverter(T))] Sep 11, 2023
@straight-shoota
Copy link
Member

I don't think annotations can reference generic type arguments.

Not sure if that's just an omission or if there are any reasons that prevent implementintg this.

@jgaskins
Copy link
Contributor Author

They can. This code compiles and runs:

annotation MyAnnotation
end

struct Foo(T)
  @[MyAnnotation(converter: OMG(T))]
  getter bar : T

  def initialize(@bar)
  end
end

pp Foo(String).new("asdf").bar
# => "asdf"

My understanding of annotations is that they're arbitrary expressions for macros to inject into your code and the compiler doesn't seem to do anything with them otherwise. Whether it actually does, I can't tell — I'm mainly going off of observed behavior.

Is there a way to output the full code in a file after all macros are expanded — like crystal tool expand, but for the entire file? I feel like that might help find out what's going on.

@straight-shoota
Copy link
Member

Right. It's not the annotation itself that errors, it's the code that's generated for it. The reported location is a bit confusing.

I believe the problem is that the converter path expands in #initialize(::DB::ResultSet) which is defined on DB:Serializable, not Event(T) and thus won't know about the generic type argument T when the compiler tries to resolve it.

@jgaskins
Copy link
Contributor Author

Oh, right, it's not defined in the constructor that gets defined in the included hook. Hmm...

@straight-shoota
Copy link
Member

straight-shoota commented Sep 13, 2023

A reduced example to demonstrate the issue looks like this:

module Foo
  def initialize
    @id = T.new # Error: undefined constant T
  end
end

class Bar(T)
  include Foo
end

Bar(String).new

In the case of Serializable, the #initialize body is defined by a macro expansion.

Projecting that on the simplified example, I'm getting code that actually works though:

module Foo
  def initialize
    {% for var in @type.instance_vars %}
      @{{ var.name }} = {{ var.type }}.new
    {% end %}
  end
end

class Bar(T)
  include Foo
  
  @id : T
end

Bar(String).new
This makes sense because due to the macro expansion in `#initialize` referring to `@type`, the method really has a different instantiation for every type. The question is, why does it not work with `DB::Serializable` which basically does a similar thing? (Same for `JSON::Serializable` and `YAML::Serializable` from stdlb).

The previous example doesn't make any sense because var.type is already expanded at this point and does not compare with the raw type argument reference.

@straight-shoota
Copy link
Member

As a workaround you should be able to replace T with an expression that resolves to the expected type in the given context.
For example:

@[DB::Field(converter: Event::Converter(typeof(raw)))]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants