Crystal: Types in annotations are resolved in the namespace of the macro instead of the annotation instance

Created on 4 Sep 2018  路  12Comments  路  Source: crystal-lang/crystal

annotation Ann
end

module UsingAnn
  def method
    {% for ivar in @type.instance_vars %}
      {{ ivar.annotation(::Ann)[0].resolve }}
    {% end %}
  end
end

class Foo
  include UsingAnn

  @[Ann(Bar)]
  getter foo : String?

  module Bar
  end
end

Foo.new.method

This code should find the type Bar as it's in the namespace where the annotation is used, but the resolve call fails because Bar is looked up in the UsingAnn namespace instead.

Found using this snippet, which fails to compile:

require "json"

class Foo
  include JSON::Serializable

  @[JSON::Field(converter: Converter)]
  getter foo : String

  module Converter
    def self.from_json(pull)
      "foo"
    end
  end
end

Foo.from_json("")

bug compiler

Most helpful comment

While @firejox's solution works in this example, it breaks as soon as someone decides to use the fully qualified name (it will look for Foo::Foo::Bar in this example). This would essentially break existing codebases circumventing this issue using the fully qualified name.

Although i appreciate how horrible the following solution is, i would like to add it in case someone finds it useful (i sincerely hope not)

annotation Ann
end

macro resolve_first(*paths)
  {{ (paths.find(&.resolve?) || paths.first).resolve }}
end

macro find_const(const)
  {% path_fragments = [""] + @type.name.split("::") %}
  {% paths = path_fragments.map_with_index { |_, i| path_fragments[0, path_fragments.size - i].join("::").id } %}
  resolve_first({{ paths.map { |path| "#{path}::#{const}".id }.splat }})
end

module UsingAnn
  def method
    {% for ivar in @type.instance_vars %}
      pp find_const({{ ivar.annotation(::Ann)[0].id }})
    {% end %}
  end
end



class Foo
  include UsingAnn

  @[Ann(Bar)]
  getter foo : String?

  @[Ann(Foo::Bar)]
  getter foobar : String?

  module Bar
  end
end

Foo.new.method

Output:

Foo::Bar
Foo::Bar

All 12 comments

I knew about this problem but didn't say anything. I don't know how it can be fixed. The problem is not just resolve, is pasting the type in the macro, which ends up in a different context (resolve isn't being used in the macro of JSON::Serialiazable).

The other problem is a converter can also be Time::Format.new("..."), and resolve won't be useful there.

I think annotations were not well thought (my fault, again). Or maybe all expressions in annotations should try to be resolved (but then I have no idea how will they "paste" correctly in macro code).

Maybe annotation could define what keys are available and whether they need to be typed or just passed as AST... just throwing ideas here.

The workaround is of course fully qualifying the type name, but it's not nice.

I guess that's one area where the annotation can be configurable:
Say sth like:

annotation JSON::Field
  param converter : Path, resolve_on_call: true
end

So that when I do @[JSON::Field(converter: Converter)] as in the given example in the OP, the macro will get Foo::Converter instead of Converter for the converter param.

@bew Yeah, but that's just adding more on top of the pile of complexity that the language is already. That's not a simple solution.

@asterite why not just resolve the full path of types as soon as you use them in annotations so it's always unambiguous. So @[Ann(Bar)] will always be recorded as @[::Ann(::Foo::Bar)]. That does mean either all types have to be defined before they're used in annotations, or that the compiler needs another pass.

I don't know, I guess someone will have to play with that and try.

Is this related to this particular issue?

require "json"

struct Enum
  class StringConverter(T)
    def self.from_json(value : JSON::PullParser) : T
      T.parse(value.read_string)
    end

    def self.to_json(value : self, json : JSON::Builder)
      json.string(value.to_s)
    end
  end
end

# Fails to compile
class Foo
  include JSON::Serializable

  enum Bar
    Baz
  end

  # undefined constant Bar
  @[JSON::Field(converter: Enum::StringConverter(Bar))]
  property bar : Bar
end

# Compiles nicely
class Foo
  enum Bar
    Baz
  end

  JSON.mapping({
    bar: {
      type:      Bar,
      converter: Enum::StringConverter(Bar),
    },
  })
end

Foo.from_json("")

With Serializable: https://carc.in/#/r/5dk7
With JSON.mapping: https://carc.in/#/r/5dk8

Yes, more or less. As a workaround use the fully qualified name.

Here's another broken JSON example, but it doesn't use the JSON::Field annotation. Does this have the same problem, or is it a different problem that I should file a new issue ticket for?

require "json"

module Namespace
  struct Foo
    include JSON::Serializable

    @bar : Bar = \
      Bar.new # ERROR: undefined constant Bar

    def initialize(@bar = Bar.new)
    end
  end
end

module Namespace # comment this out, and the example compiles
  struct Bar
    include JSON::Serializable

    @s : String = ""

    def initialize(@s = "s")
    end
  end
end

Namespace::Foo.from_json("{}")

@jemc It's a related issue. The problem is that JSON::Serializable works by defining an initialize method in JSON::Serializable so Bar.new isn't found relative to that location. One way to solve this could be to move the initialize method to the type that includes JSON::Serializable (using an included macro hook) but the problem wouldn't be entirely solved because it could then break for subclasses (unless we keep copying the initialize method somehow).

I don't know yet what the solution to this problem is.

Maybe JSON.mapping wasn't that bad after all :-)

How about use @type.constant? It can lookup the type with relative path.

annotation Ann
end

module UsingAnn
  def method
    {% for ivar in @type.instance_vars %}
      {{ @type.constant(ivar.annotation(::Ann)[0].id) }}
    {% end %}
  end
end

class Foo
  include UsingAnn

  @[Ann(Bar)]
  getter foo : String?

  module Bar
  end
end

pp Foo.new.method

https://play.crystal-lang.org/#/r/896d

While @firejox's solution works in this example, it breaks as soon as someone decides to use the fully qualified name (it will look for Foo::Foo::Bar in this example). This would essentially break existing codebases circumventing this issue using the fully qualified name.

Although i appreciate how horrible the following solution is, i would like to add it in case someone finds it useful (i sincerely hope not)

annotation Ann
end

macro resolve_first(*paths)
  {{ (paths.find(&.resolve?) || paths.first).resolve }}
end

macro find_const(const)
  {% path_fragments = [""] + @type.name.split("::") %}
  {% paths = path_fragments.map_with_index { |_, i| path_fragments[0, path_fragments.size - i].join("::").id } %}
  resolve_first({{ paths.map { |path| "#{path}::#{const}".id }.splat }})
end

module UsingAnn
  def method
    {% for ivar in @type.instance_vars %}
      pp find_const({{ ivar.annotation(::Ann)[0].id }})
    {% end %}
  end
end



class Foo
  include UsingAnn

  @[Ann(Bar)]
  getter foo : String?

  @[Ann(Foo::Bar)]
  getter foobar : String?

  module Bar
  end
end

Foo.new.method

Output:

Foo::Bar
Foo::Bar
Was this page helpful?
0 / 5 - 0 ratings