Crystal: Incorrect scope after including **::Serializable

[z64]

Given:

require "json"
require "json/next/serialization" # (required as I'm using docker nightly; "0.25.0+9")

module Options
end

class Foo
  include JSON::Serializable
  include Options
end

Foo.new

Yields:

Error in app.cr:9: JSON::Serializable::Options is not a module, it's a annotation

  include Options
          ^~~~~~~

Compiles if Serializable is included last (or if Options is renamed to something not taken):

require "json"
require "json/next/serialization"

module Options
end

class Foo
  include Options
  include JSON::Serializable
end

Foo.new

Crystal 0.25.0+9 [bb8968476] (2018-06-14)

LLVM: 4.0.0
Default target: x86_64-unknown-linux-gnu

Not fully sure of the scope of the issue (is it with annotations? macro included?), this is just the case I ran into this with.

[kostya]

this is because exist annotation JSON::Serializable::Options it conflict with your module, try:

class Foo
  include JSON::Serializable
  include ::Options
end

[z64]

Yeah I knew that would fix it too, forgot to mention.

I guess I didn't realize that annotations come along with include like that and that the compiler wouldn't skip annotations because I was trying to include.

[RX14]

Feels to me like we shouldn't be including stuff into people's classes when they include JSON::Serializable.

Perhaps a rename to JSON::SerializableOptions and similar for the other misc modules and annotations under JSON::Serializable.

[straight-shoota]

@RX14 I'd question if annotations should be included into other namespaces at all...? I can't think of a use case for this.

[straight-shoota]

But yes, removing Options from the includeable JSON::Serializable namespace would be a quick fix that doesn't need to change the compiler.

JSON::Serializable::Strict and JSON::Serializable::Unmapped similarly shouldn't be duplicated into namespaces where JSON::Serializable is included.

[asterite]

Well, if annotations would have been named OptionAnnotation we woudn't be having this discussion... 🙄

(however, it of course can happen with any other type, not just annotations)

Just learn to live with these deficiencies of the language, they are not a big deal.

[kostya]

Yes, this is just as any other modules conflicts. I think this is ok, and not even related to annotations, it happens from time to time.

[RX14]

@straight-shoota include should include everything in the target module. The problem is that we've created a module designed to be included then used it also for namespacing, which is incorrect.

[straight-shoota]

How about we put Options, Strict and Unmapped in a new module JSON::Serialization? Which doesn't do anything else, it's just a namespace.

[kostya]

this would look weird:

  struct A
    include JSON::Serializable
    include JSON::Serialization::Unmapped
    @a : Int32
  end

[asterite]

Think about all the people wanting to include both Serializable and a custom module named Options or Unmapped. The count will be maybe just two or three people in the universe. I think it's better those two or three add a full path there, instead of redesigning the API and ruin it for everyone else (including those two or three).

[bcardiff]

I neither there is much to do here.

@straight-shoota if a module is used for namespaces lots of annotation you want them to be included in the namespace to avoid repetition of the full path.

As @RX14 pointed out is a side effect of including Serializable inside Foo that hides the top level Options. User will need to type ::Options.

Whatever other nice name we choose for Options will conflict with some user.

[kostya]

this is worse conflict, btw:

record Link, before : String?, href : String?, anchor : String?, after : String?

Error in 1.cr:1: expanding macro

record Link, before : String?, href : String?, anchor : String?, after : String?
^
...

Link is not a struct, it's a annotation

[asterite]

Vote to make annotation names end with a mandatory Annotation suffix? :+1:

[kostya]

may be Annotation suffix not bad, because defining annotations expect to be quite rare.

[asterite]

And also, their usage is only inside @[...], and when fetching via macro method get_attribute, so it's almost like a separate namespace without having one. And, again, this is _almost_ like C# implements annotations.

[Sija]

@asterite Annotations::Link sounds not that bad too + it's a single namespace for all Annotations.

[asterite]

@Sija What about JSON::Serializable? Does it become Annotations::JSON::Serializable? JSON::Annotations::Serializable?

The first PR I sent with annotations was about having:

module Moo
  annotation FooAnnotation
  end
end

And using it like:

@[Moo::Foo]

And retrieving in macros with:

type.get_annotation(Moo::FooAnnotation)

Since annotations are usually created by the same ones consuming them (like JSON::Serializable is), it's the same amount of typing for users (they'd still have to write @[JSON::Field]), and only slightly more inconvenient for authors (having to define annotation FieldAnnottion and doing .get_annotation(JSON::FieldAnnotation)), while also completely avoiding the names clash. And it also makes it obvious that if you see LinkAnnotation in the docs, it's dead obvious that it's an annotation, as opposed to the current Link, Flags, Primitive, and other top-level annotations.

And this is proven to work, it's how C# implements it (kind of).

I'll just leave this idea here. You can discuss other alternatives, and eventually do a change if it's needed 👍

[straight-shoota]

What about annotation Field and .get_annotation(JSON::Field) being rewritten to annotation FieldAnnotation and .get_annoation(JSON::FieldAnnotation) automatically by the compiler? It would be fairly straightforward, and shouldn't lead to many surprises while keeping the short names.

[RX14]

I still don't like it because it's magic and a hack. If we're going to jump through all these hoops, why not actually have a separate namespace for annotations.

And implementing this doesn't solve the problem, because JSON::Serializable contains other modules (JSON::Serializable::Strict) as well as other annotations, which will conflict in exactly the same way as annotations. In fact it will be worse, because they will be included by include and actually work, silently possibly accidentally adding features to your class.

See: https://carc.in/#/r/4b0v

[bcardiff]

The weird part of Annotation suffix is that either

a) the declaration is annotation FooAnnotation and ussed as foo @[Foo] leaving references in macros/code as FooAnnotation since that is the name of the constant.

or

b) the declaration is annotation Foo and ussed as foo @[Foo] and leaving references in macros/code as FooAnnotation, making it more implicit but harder to understand at a first glance and will obviously clash with FooAnnotation constant.

I'm ok with going through option b. The only one that should care about that is the developer coding the effect of the annotation. So it's easy to point in the documentation/guide. It helps for better annotation name that won't clash with other constants.

[asterite]

I think a is better. It's how it's done in C#

[straight-shoota]

I guess as long as it is used as @[Foo], calling the annotation definition Foo (a) or FooAnnotation (b) makes no big difference. Both cases can work and mostly bother annotation developers. It doesn't really matter IMHO. Docs for users can refer to the name transparently.

[bcardiff]

(a) it's like it's done in C# because they are classes. But here they word annotation is already stated. That is way I am more in favor of (b). I am not against (a) though. The whole thing is reducing the clashing which both do.

[asterite]

I actually think the current behaviour is fine. It's no use to open an issue and complain about every little detail in the language. The solution was given (use ::Options). Otherwise the language will never settle.

[bew]

I actually think the current behaviour is fine

@asterite I disagree

IMO the annotations should not be part of the user-accessible type system.
As they can only be used in @[..] and macro's get_annotation(..), it makes no sense to make them conflict with other types/constants.