User-defined annotations #6063

asterite · 2018-05-04T22:27:46Z

This PR adds user-defined annotations to the language.

Annotations are the old attributes:

@[Link("sqlite3")] # <- this is an annotation 
lib LibSQLite3
end

I chose to use the name "annotation" instead of "attribute" because the latter is much more common in code: I searched usage of "annotation" and it was all over the place in Crystal projects, whereas I could just find one project that used the name "annotation" somewhere. So to avoid breaking a lot of projects, "annotation" might be a better name.

It works like this.

First you define an annotation (just like any other type):

module JSON
  # Put this on top of instance variables to control serialization
  anntotation Field
  end
end

Note that annotation now becomes a keyword, just like class and def, so it's prohibited mostly everywhere, so that's why I chose to go with that name, because it's pretty rare.

Once you define an annotation, you can use it:

class Point1D
  @[JSON::Field(key: "ex")] 
  @x : Int32
end

And then you can query it:

class Point1D
  def do_something
    {% for ivar in @type.instance_vars %}
      {% ann = ivar.annotation(::JSON::Field) %}
      {% key = ((ann && ann[:key]) || ivar).id %}
      # do something with key
    {% end %}
  end
end

In the above example it looks like JSON serialization becomes a lot more tedious than the current JSON.mapping. However, the way I imagine it, you would include a JSON::Serializable module, which would define the initialize and to_json methods, and by default will use all instance variables, even if they don't have the JSON::Field annotation on them. The annotation is only used to control the serialization, like choosing the key, or whether to ignore a field.

You can compile the compiler with this PR and try it out, here's a working prototype.

Annotations can also be placed on top of types, and later queried. Methods for now can't have (user-defined) annotations, but maybe in the future they will.

The benefits of having annotations is that it provides a way to attach metadata to instance variables. Right now one has to use a compile-time constant, add info to that, and later process it in a finished macro, which isn't ideal (though depending on what you want to do, because of this chicken and egg problem, you might need to still do it like that).

Other benefits, which I will highlight just by using the JSON::Serialization implementation using annotations:

it works well with inheritance and included modules
you can make a record include JSON::Serializable and not have to repeat the JSON.mapping declaration

Of course this will also apply to YAML serialization and any other formats. And I'm sure you will come up with great ways to use annotations.

Now, the annotation declaration is empty right now:

annotation Foo
  # Nothing can go in here
end

In the future, we could specify what things an annotation can be applied to, or what are the fields it can hold. But since it's mainly a compile-time thing that you'll interact with in macros, and macros are pretty lax (they feel like a dynamic Crystal), it makes sense not to restrict that. That's why you can still specify a converter like @[JSON::Field(converter: Time::Format.new("%F %T"))], where it's not clear what type converter has (like this, anything is basically an ASTNode, which is what macros work with).

A few other things:

this PR fixes a bug related to getting the default value of an instance variable at compile-time (related to inheritance and modules)
this PR adds the TypeNode#nilable? macro method, which I think is very useful (and used in my prototype json serializable annotation)
the prototype has a hack somewhere: I tried to fix it but I couldn't understand why it failed. So for now, we can use that workaround, maybe later with time and patience someone can fix it
someone will have to update the current docs regarding all of this ;-)

If we merge this, just in the next release we'll be able to define the JSON::Serializable module and all that stuff. We could do it now by using compare_version and defining stuff conditionally based on the next compiler version, but I'd rather wait for a release to be able to try and review stuff more easily.

See also: #3620, #3325, #5643

bew

Really nice PR 😄

bew · 2018-05-04T23:21:05Z

src/compiler/crystal/types.cr

@@ -162,6 +162,10 @@ module Crystal
      self.is_a?(NilType)
    end

+    def nilable?
+      self.is_a?(NilType) || (self.is_a?(UnionType) && self.union_types.any?(&.nil_type?))


Isn't it self.union_types.any?(&.nilable?)?

A union type can only have single types, it doesn't recurse (or it doesn't need to recurse).

Oh ok, didn't know that

RX14 · 2018-05-04T23:44:19Z

I'm definitely not liking the mismatch of naming between annotations definition and their usage in code. It's confusing and has no presedent. If it takes a massive hack in the compiler (hardcode Primitive until next release) to do it without having to suffix annotations with Annotation, it's definitely worth it to me.

The rest is great.

asterite · 2018-05-05T00:17:24Z

@RX14 As I explain somewhere above, I was actually going to do it that way (with the Annotation suffix) to avoid cluttering the top-level namespace. Then I said "Meh, let's leave that for later" and then I immediately found a conflict, which only means that avoiding that name clash is actually something good.

Plus, again, it's how it's done in C# (only in C# you can use the full attribute name too, when using it, but I don't think we need that in Crystal, or at least we could maybe easily support it later).

But I wouldn't mind not using the suffix and waiting a bit. It's just that seeing Link there in the API docs, openning and seeing "well, it's a link attribute, not a link class or struct or module" is probably a bit confusing. It's much clear if it's called LinkAttribute. The example I gave about XML::Element and XML::ElementAttribute is algo a good one. And in my JSON::Serializable prototype there is a JSON::SerializableAttribute too, to specify strictness of parsing, and it just looks nice and consistent.

Well, that's my argument anyway :-)

oprypin · 2018-05-05T00:35:54Z

I agree with RX14.

I don't know about conflicts, maybe it's good to prevent having a class and an annotation under the same name?

Or you could just make a hack that appends Annotation to all names, both in annotation and @[ ] (the latter is actually already done!)

oprypin · 2018-05-05T00:38:47Z

Your argument about discerning annotations in docs is not an argument towards particular naming, it's really just a feature request for docs generator.

asterite · 2018-05-05T00:41:16Z

That said, if everyone else also dislikes this "hack", for now, I can hardcode that "Primitive" always refer to the primitive annotation, when used in annotations, and later remove that rule.

kostya · 2018-05-05T00:53:40Z

thanks, this json serialization, i dream about:

class JsonWithDefaults
  include JSON::Serializable

  property a = 11
  property b = "Haha"
  property c = true
  property d = false
  property e : Bool? = false
  property f : Int32? = 1
  property g : Int32?
  property h = [1, 2, 3]
end

if we also can generate initialize with named_tuple it would be super.

asterite · 2018-05-05T01:08:29Z

@kostya that's possible. What do you mean?

kostya · 2018-05-05T01:14:06Z

class JsonWithDefaults
  include Initialize
  include JSON::Serializable

  property a : Int32
  property b = "Haha"
end

JsonWithDefaults.new(a: 5, b: "bla")
JsonWithDefaults.new(a: 5)
JsonWithDefaults.from_json(%<{"a":5}>)

asterite · 2018-05-05T01:19:37Z

Ah, yes, that's coming soon, too. Actually, it can already be done with the addition of knowing the default value of an instance var, only that the error message if used incorrectly are a bit ugly, but nothing too terrible.

asterite · 2018-05-05T02:15:29Z

Please add an emoji in this comment for:

👍 to keep the requirement of the "Annotation" suffix
👎 to remove the requirement of the "Annotation" suffix

But if we go with 👎 , we'll have to think of another name of the attribute used on top of a type to let the parsing be strict (though now that I think about it, there was this idea of having a method you could override to handle unknown attributes, which by default would just pull.skip... so I guess this is also a 👍 for 👎 from my part 😛).

asterite · 2018-05-05T11:34:47Z

I just pushed a commit that removes the requirement that annotation names must end with "Annotation". I had to hardcode two things in annotations, Primitive and Flags, because there's Int::Primitive and File::Info::Flags, and there isn't yet a way to write ::Primitive and ::Flags in annotations, but in the next version we can use that and remove the hardcoded rule.

RX14 · 2018-05-05T11:40:32Z

src/compiler/crystal/semantic/semantic_visitor.cr

+    # just yet in annotations, we temporarily hardcode
+    # that `Primitive` inside annotations means the top
+    # level primitive.
+    # We also have the same problem with File::Flags, which


Could you put a TODO: string somewhere in this comment here so we can grep for all todos and not miss this?

wooster0 · 2018-05-05T12:18:06Z

src/enum.cr

@@ -26,7 +26,7 @@
 #
 # ### Flags enum
 #
-# An enum can be marked with the `@[Flags]` attribute. This changes the default values:
+# An enum can be marked with the `FlagsAnnotation` annotation. This changes the default values:


I think FlagsAnnotation should be changed to Flags here.

Thanks! I reverted this change because annotations won't have (nice) docs for now, so better not to link them just yet.

bew · 2018-05-05T16:20:28Z

src/compiler/crystal/program.cr

-      types["RaisesAnnotation"] = @raises_annotation = AnnotationType.new self, self, "RaisesAnnotation"
-      types["ReturnsTwiceAnnotation"] = @returns_twice_annotation = AnnotationType.new self, self, "ReturnsTwiceAnnotation"
-      types["ThreadLocalAnnotation"] = @thread_local_annotation = AnnotationType.new self, self, "ThreadLocalAnnotation"
+      types["AlwaysInline"] = @always_inline = AnnotationType.new self, self, "AlwaysInline"


So annotations conflicts with top level types? I would have thought they are in a separate category instead. Is it temporary?

bew · 2018-05-05T16:24:05Z

So builtin annotations conflicts with top level types? I would have thought they are in a separate category

asterite · 2018-05-05T16:34:25Z

They are in the same namespace, yes. Maybe later we can put them in a separate namespace, I don't know. But then, if they are in a different namespace, how would you show them in docs? That's a bit ugly.

That's why I first suggested with stick with the "Annotation" suffix, but nobody liked that. But I don't think it's a big deal, really.

bew · 2018-05-05T16:50:17Z

We could put them in a special Crystal::Annotation namespace, and the doc generator would just have to check in this namespace

asterite · 2018-05-05T17:05:18Z

But annotations can appear under other namespaces, like in the JSON::Field example above. And you can define top-level annotations if you want to.

asterite · 2018-05-05T17:17:15Z

@kostya Hey, I found a way to do what you want with the current (0.24.2) version of Crystal 😄

class Object
  module Initialize
    def initialize(**args : **T) forall T
      {% for key in T.keys.map(&.id) %}
        {% unless @type.instance_vars.map(&.id).includes?(key) %}
          {% raise "no argument named '#{key}'" %}
        {% end %}

        @{{key}} = args[{{key.symbolize}}]
      {% end %}
    end
  end
end

class Foo
  include Initialize

  property id : Int32
  property x = 1
  property y = "foo"
end

p Foo.new(id: 1)
p Foo.new(id: 2, y: "hi")
p Foo.new(id: 2, x: 3)
p Foo.new(id: 3, x: 10, y: "hello")

And you even get a "nice" error message if you use it in a wrong way:

Error in bar.cr:27: no argument named 'z'

p Foo.new(z: 10)
      ^~~

bew · 2018-05-05T17:41:04Z

@asterite true, what about keeping the Annotation suffix internally (but keep annotation Field & @[Field] clean)

asterite · 2018-05-05T17:56:43Z

Then it will clash with stuff that ends with Annotation.

Let's leave it like this, there's no problem, really.

kostya · 2018-05-05T18:08:51Z

bug: https://play.crystal-lang.org/#/r/40ab (easy fixable), also when mismatch type bad error message: https://play.crystal-lang.org/#/r/40ao

RX14 · 2018-05-05T18:24:19Z

That mismatched type error message looks fine to me.

RX14

This is good to merge though.

asterite · 2018-05-08T21:30:00Z

Things to specify can be:

Where the annotation can be used (instance var, method, etc.)
What's the shape of the annotation arguments (for example one could specify the named argument names at least, so you'll get a fast and nice error message when used incorrectly)

RX14 · 2018-05-09T00:16:05Z

Let's get a new JSON interface based on this PR merged, then we can cut 0.25 I think.

Sija · 2018-05-09T00:44:39Z

@RX14 Based on the sheer amount of features it should be more like 0.30 🍘

bew · 2018-05-09T01:07:50Z

@RX14 iirc you can't directly use the compiler feature until next release, so the new JSON interface will have to wait next release

bew · 2018-05-09T01:09:50Z

But maybe we can release a 0.24.3 with annotations (only?), so that we can develop that new serialization interface for 0.25 ?

asterite · 2018-05-09T01:38:20Z

This can be done. If someone wants to work on this, let us know.

Here's a diff to do it:

diff --git a/src/json.cr b/src/json.cr
index ce859e2d7..58abfd904 100644
--- a/src/json.cr
+++ b/src/json.cr
@@ -93,3 +93,7 @@ module JSON
 end
 
 require "./json/*"
+
+{% if compare_versions(Crystal::VERSION, "0.25.0") >= 0 %}
+  require "./json/next/serialization"
+{% end %}
diff --git a/src/json/next/serialization.cr b/src/json/next/serialization.cr
new file mode 100644
index 000000000..0b14ba494
--- /dev/null
+++ b/src/json/next/serialization.cr
@@ -0,0 +1,7 @@
+module JSON
+  annotation Field
+  end
+
+  module Serializable
+  end
+end

In short:

We conditionally require a file based on the crystal version
We use the new features in this file

I put it in json/next/serialization.cr because json.cr does require "./json/*" so we must put it in a nested directory.

For specs it'll be a bit trickier.

Alternatively, there's no reason why we can't release 0.25.0 "soon", and then work on 0.26.0 next and release it soon too. I used to released maybe once a month in the past. We can release as often as we'd like (except that I'm not in charge of releasing anymore).

kostya · 2018-05-09T06:10:12Z

i add some initial work on it: #6082

straight-shoota · 2018-05-09T07:20:44Z

It's been too long since last release... I thought the release process was supposed to be easier now 🤷‍♂️

RX14 · 2018-05-09T10:06:18Z

Release is easier but it still needs someone from manas. @bcardiff should be around in a week or so to cut a release...

RX14 · 2018-05-09T10:08:43Z

Maybe we should start a release PR now though, to track anything left we want in 0.25.0 and write the changelog.

asterite · 2018-05-09T11:11:59Z

Can we ask them to give you, RX14, or someone else from the core team, the ability to do releases? At this point this is the only thing that's holding us back from managing Crystal as a true open source community project.

RX14 · 2018-05-09T12:04:55Z

@asterite yeah I think thats a good idea. I'd really like crystal to be disconnected from manas in terms of management. Perhaps I'll ask @bcardiff to walk me through next release?

User-defined annotations

d6f1c61

asterite self-assigned this May 4, 2018

bew reviewed May 4, 2018

View reviewed changes

Don't require annotations to end with "Annotation"

96a658d

RX14 requested changes May 5, 2018

View reviewed changes

Add TODO about the hardcoded annotation rules

1d793af

wooster0 reviewed May 5, 2018

View reviewed changes

Don't change enum docs for now

8cd1ad0

bew reviewed May 5, 2018

View reviewed changes

Sija mentioned this pull request May 6, 2018

[Feature Request] Add bitfields to C structs #3898

Closed

RX14 approved these changes May 8, 2018

View reviewed changes

In macros, annotation nests

615adac

asterite merged commit 4360c69 into crystal-lang:master May 8, 2018

asterite added kind:feature topic:compiler labels May 8, 2018

asterite added this to the Next milestone May 8, 2018

asterite mentioned this pull request May 8, 2018

Meta attribute #3620

Closed

straight-shoota mentioned this pull request Sep 7, 2018

JSON.mapping in subclass does not inherit it's parent's mapping #3325

Closed

Heaven31415 mentioned this pull request Oct 14, 2018

Document usage of annotations crystal-lang/crystal-book#292

Closed

artworx mentioned this pull request Oct 16, 2018

Add support for annotations vim-crystal/vim-crystal#57

Open

Heaven31415 mentioned this pull request Oct 16, 2018

Add support for annotations crystal-lang-tools/vscode-crystal-lang#68

Closed

straight-shoota mentioned this pull request Oct 16, 2018

Update crystal.js highlightjs/highlight.js#1875

Merged

akadusei mentioned this pull request Oct 19, 2018

add an "overrides" annotation #1647

Open

j8r mentioned this pull request Apr 17, 2019

TypeNode#annotated_types #7648

Closed

This was referenced May 2, 2019

Add user defined annotations crystal-lang-tools/language-crystal#27

Merged

Add support for annotations crystal-lang-tools/scry#156

Merged

HertzDevil mentioned this pull request Nov 20, 2020

Replace attributes (e.g. TypeNode#has_attribute?) with annotations #9943

Closed

straight-shoota mentioned this pull request Mar 15, 2022

Use @[::Primitive] and @[::Flags] where necessary #11900

Merged

User-defined annotations #6063

User-defined annotations #6063

Conversation

asterite commented May 4, 2018 • edited Loading

bew left a comment

Choose a reason for hiding this comment

bew May 4, 2018

Choose a reason for hiding this comment

asterite May 4, 2018

Choose a reason for hiding this comment

bew May 4, 2018

Choose a reason for hiding this comment

RX14 commented May 4, 2018 • edited Loading

asterite commented May 5, 2018

oprypin commented May 5, 2018

oprypin commented May 5, 2018

asterite commented May 5, 2018

kostya commented May 5, 2018

asterite commented May 5, 2018

kostya commented May 5, 2018

asterite commented May 5, 2018

asterite commented May 5, 2018

asterite commented May 5, 2018

RX14 May 5, 2018 • edited Loading

Choose a reason for hiding this comment

asterite May 5, 2018

Choose a reason for hiding this comment

wooster0 May 5, 2018

Choose a reason for hiding this comment

asterite May 5, 2018

Choose a reason for hiding this comment

bew May 5, 2018

Choose a reason for hiding this comment

bew commented May 5, 2018

asterite commented May 5, 2018

bew commented May 5, 2018

asterite commented May 5, 2018

asterite commented May 5, 2018

bew commented May 5, 2018

asterite commented May 5, 2018

kostya commented May 5, 2018 • edited Loading

RX14 commented May 5, 2018

RX14 left a comment

Choose a reason for hiding this comment

asterite commented May 8, 2018

RX14 commented May 9, 2018

Sija commented May 9, 2018

bew commented May 9, 2018

bew commented May 9, 2018

asterite commented May 9, 2018

kostya commented May 9, 2018

straight-shoota commented May 9, 2018

RX14 commented May 9, 2018

RX14 commented May 9, 2018

asterite commented May 9, 2018 • edited Loading

RX14 commented May 9, 2018

asterite commented May 4, 2018 •

edited

Loading

RX14 commented May 4, 2018 •

edited

Loading

RX14 May 5, 2018 •

edited

Loading

kostya commented May 5, 2018 •

edited

Loading

asterite commented May 9, 2018 •

edited

Loading