TypeNode#annotated_types

[Blacksmoke16]

Returns an array of all types (struct/class/module) with the given annotation. Addresses some of #7274.

annotation Foo
end

@[Foo(val: 1)]
class Bar
end

@[Foo(val: 2)]
class Baz
end

class Blah
end

{% for a in Foo.annotated_types %}
  pp {{a.annotation(Foo)[:val]}}
{% end %}

# => 1
# => 2

A future PR could add #annotated_methods/#annotated_variables, but #annotated_types is the more useful one IMO.

/cc @vladfaust

[vladfaust]

Small note – you don't actually check for types in specs, only for their size. I'm also concerned whether if it works with #7326?

[Blacksmoke16]

In the case of #7326 you actually want to keep duplicates since the values on each could be different. However, annotated_types you only want the list of types that are annotated, you wouldn't want Klass to show up twice just because you added two annotations to it.

Even if those two annotations had separate values, you would still be able to use #annotations in the for loop to get access to them.

[j8r]

This can be confused with classic type annotations. The choice over Attributes was decided in #6063

[asterite]

Just a note that things like this push us further and further away from incremental/modular compilation (at this point we should probably drop that idea altogether and accept the existing nature of Crystal).

[j8r]

@asterite if each library/shard is independent from each other, won't it be possible to compile each one and link them to the main program?

[Blacksmoke16]

@asterite @straight-shoota anything else need to be done?

[RX14]

@asterite incremental macros just means careful tracking of exactly which AST nodes a macro "sees" and being able to verify they're unchanged. I think that can be automated fairly well. We should remove the main blocker for incremental/modular compilation (type restrictions only being restrictions) before blocking other features over that though.

I think this is good to go.

[bcardiff]

I would like to have the ability to query types/ivars/methods given an annotation type.

I think that the incremental is doable (at some cost).

But what I would like to have a discussion/design before merging this is regarding subtyping. If Bar < Foo, are annotations inherited or not? Would that behavior be better specified in the annotation declaration itself (like in C# IIRC) or to define that upon querying (adding params to the proposed API)

[Blacksmoke16]

I think it would be better if they were not, and just added to each type explicitly; however I could be convinced otherwise. IMO, this would make it less likely for unintended things to happen. Being more verbose but allowing for more control seems to be a fair trade to me.

[Blacksmoke16]

Renamed that property @straight-shoota, CI failure seems unrelated?

[Blacksmoke16]

I don't suppose this will make it into 0.29.0?

[bcardiff]

@Blacksmoke16 I am interested in the feature but I haven't invested time yet to have proper design discussions. From my past experience, I am still inclined to mimic C# interface. But we should also consider how other language features impact: an annotation that is applied in a module that is included in a class does contribute to the annotations?

[kalinon]

I would be very interested in this feature as well

[Blacksmoke16]

I'd like to get the discussion around this started again. I'm getting close to a point where this is kind of a blocker to some Athena stuff I'm working on. #8133 will be a good temporary solution, but having both this and that would be ideal.

[watzon]

Any movement on this? This really is probably the ideal way for annotations to work, since they won't be hamstrung by needing to loop over annotations inside of a class. I really need this working for a project as well.

[kalinon]

Any movement on this? This really is probably the ideal way for annotations to work, since they won't be hamstrung by needing to loop over annotations inside of a class. I really need this working for a project as well.

Yes +1 for this feature

[asterite]

I think this will be needed for the new logging stuff, otherwise how do you find all registered logs?

(I wonder why there's no registry and instead the proposal is using annotations, but it's a separate issue).

[watzon]

It does seem necessary for a number of things, and it's so close to approval

[bcardiff]

@asterite it's not needed. The following works.

  def self.build_backend(type : String, config : Backend::Config)
    {% begin %}
      case type
      {% for t in Log::Backend.all_subclasses %}
        {% if reg = t.annotation(Log::RegisterBackend) %}
          when {{reg[0]}}
            {{t}}.new(config)
        {% end %}
      {% end %}
      else
        raise "There is no Log::Backend with @[Log::RegisterBackend(\"#{type}\")] annotation"
      end
    {% end %}
  end

It is not a registry as DB drivers because that might lead to a situation where the backend is not registered on time to initialize the Log constants.

Back to coding ... 👋

[asterite]

So the annotation is only for the name, but you still need to inherit Log::Backend. Interesting!

Why not rely on a class method like backend_name? Why use annotations which are not type-safe?

[asterite]

Also a warning: if a generic type inherits Log::RegisterBackend then it will stop working because you won't be able to use that uninstantiated generic type. Maybe there's no use case for generic types, though...

[bcardiff]

Why not rely on a class method like backend_name? Why use annotations which are not type-safe?

I liked the registry. The case is generated with literals. It can be changed of course, it is not the more important aspect :-). But the annotation based registry is doable.

[Blacksmoke16]

Also a warning: if a generic type inherits Log::RegisterBackend then it will stop working because you won't be able to use that uninstantiated generic type

This is what #8458 would help with, as it could print Foo(T) as Foo.

@asterite it's not needed. The following works.

It works yes, but IMO it seems less than ideal to have to have a parent type/module just to do this; hence this PR. Annotating a type should be enough, not for the Log use case specifically but are cases where it would be nice.

[straight-shoota]

@Blacksmoke16 Totally agree. But I don't think anyone made the argument that this PR wasn't useful because there are simple alternatives. It's just not strictly necessary for the Log::Backend example. 👍

[Blacksmoke16]

As a workaround for now, I realized you could just do Object.all_subclasses.select &.annotation(MyAnn)

[Blacksmoke16]

Closing as you can just use the workaround as defined in #7648 (comment).