Add Redefine and Override annotations

[asterite]

This PR introduces two annotations:

• @[Redefine]: when you redefine a method, that is, you replace an existing method with another one
• @[Override]: when you define a method in a subtype that's present up in the type hierarchy

You can find examples in the specs in the diff.

The semantic is not that straightforward. Let's consider each annotation.

Redefine

I first named this Overwrite but I think it would be very confusing to have Override and Overwrite, and Redefine is much clearer.

This is this case:

class Foo
  def foo(x)
  end
end

class Foo
  @[Redefine]
  def foo(x)
  end
end

The above example compiles fine: same method name, same arguments and same restrictions (none). But what if there are some type restrictions? We can have:

# 1. Redefines 👍 
# before
def foo(x)
# after
def foo(x : Int32)

# 2. Doesn't redefine 👎 
# before
def foo(x : Int32)
# after
def foo(x)

# 3. Doesn't redefine 👎 
# before
def foo(x : Int32)
# after
def foo(x : String)

In case 1 it redefines because the previous method allowed an Int32 as an argument, and now we are redefining that behaviour. We are not redefining it for all of the types, but if it happens for at least one of them then the annotation applies.

(maybe later we can introduce a flag on the attribute to allow partial redefines like this or not, but I don't know if it's necessary)

In case 2 it doesn't redefine because if we pass an Int32, the compiler will still choose the previous definition.

In case 3 the type restrictions are different so it doesn't redefine.

Override

This is this case:

class Foo
  def foo(x)
  end
end

class Bar < Foo
  @[Override]
  def foo(x)
  end
end

The above example compiles fine: same method name, same arguments and same restrictions (none). But what if there are some type restrictions? We can have:

# 1. Overrides 👍 
# ancestor
def foo(x)
# self
def foo(x : Int32)

# 2. Overrides 👍 
# ancestor
def foo(x : Int32)
# self
def foo(x)

# 3. Doesn't override 👎 
# ancestor
def foo(x : Int32)
# self
def foo(x : String)

In case 1 it overrides: there's a stricter type restriction so if we invoke Bar.new.foo(1) it will call the method in Bar and not in Foo.

In case 2 it also overrides, unlike the Redefine case, because it's in a subtype: if we pass an Int32 to Bar#foo then it will consider that method (there's no multiple dispatch here).

In case 3 the restrictions are different so it doesn't overwrite.

Caveats

Now for the caveats: I don't consider this implementation complete, but just because the whole language and all of its semantic implementation (or even definition) is incomplete. For example, right now we can redefine a method with a different argument name:

class Foo
  def foo(x)
  end
end

class Foo
  # This actually redefines the above method
  def foo(y)
  end
end

But if we invoked the first method like foo(x: 1) then now it won't compile... so in short, we are not taking names into account when redefining methods or when considering overloads. But this is a discussion to be held somewhere else.

Then, return types are also not taken into account when overriding/redefining, so here it's fine to do that and change the return type, it will compile just fine. But that's also a discussion to be held somewhere else.

In the worst scenario, if you try to use these annotations and there's a bug or limitation in the compiler preventing you to use them, you just don't use them until these bugs/limitations are fixed. Nobody is forcing you to use them (but they are useful to be more explicit about what you are trying to do).

How it relates to docs

This PR doesn't modify the way docs are generates. I'd like to tackle that in a separate PR. Basically, if you redefine or override a method and don't provide additional doc comments, the doc generator should use the other method's docs. For this we'll have to store the "other" method somewhere, but doing so will make the compiler use more memory, so I'd like to find a better way to do that, maybe store these relationship outside of the AST nodes, so that the memory is only used when needed (I'm thinking a Hash could be enough). Also, we'll have to repeat the logic of finding the redefined/overridden method, so I'd like to refactor that too in a next PR.

---

Fixes #1647

[asterite]

I forgot to mention, but the usefulness of these annotations are:

• making sure you redefine/override something and that you don't have typos when doing so
• a clearer code intent

[asterite]

No idea why one of the CI failed:

Nil assertion failed at /mnt/spec/std/signal_spec.cr:45 (Exception)

  from src/nil.cr:107:5 in 'not_nil!'

  from spec/std/signal_spec.cr:0:23 in '->'

  from src/signal.cr:255:3 in '->'

  from src/signal.cr:255:3 in 'process'

  from src/signal.cr:198:9 in '->'

  from src/fiber.cr:255:3 in 'run'

  from src/fiber.cr:75:34 in '->'

  from ???

FATAL: uncaught exception while processing handler for CHLD, exiting

[straight-shoota]

I think I've seen that CI exception before...

[Heaven31415]

In my humble opinion case 2 of override is confusing and it will get mixed up with case 2 for redefine.

I think that if someone had a really strong type restriction in ancestor, he will want to keep it for subtypes, not make it weaker.

Also you will get a really nice rule about this kind of situations: tt won't work if you go from less restrictive type to more restrictive type. Grasping this for people will be a lot easier and language won't get complicated and will still feel very light.

[asterite]

The thing is that case 2 is already being used in the compiler:

class ASTNode
  def restriction_of?(other : ASTNode, owner)
    self == other
  end
end

class Self < ASTNode
  # I want to say that this overrides a case for the `restriction_of?` method and I want
  # to make sure I didn't screw it up by having a typo or wrong number of arguments
  @[Override]
  def restriction_of?(type : Self, owner)
    true
  end
end

That's the reason case 2 works like that: if I myself can't use it in the compiler what's the point? :-)

[vladfaust]

I don’t like the syntax. Square brackets with at symbol look awful. It’s about annotations themselves, I guess. Happy for extra-verbose functionality, though!

BTW, C# uses brackets only, without @. Wouldn't it look better?

[straight-shoota]

@vladfaust This PR isn't the place to discuss annotation syntax. That's been around for a long time.

[straight-shoota]

@asterite Our of curiosity, how does the OverrideChecker perform? I suspect it shouldn't have much impact on compilation time.

[asterite]

I guess it depends on the number of annotations. I added a few and it was like 0.04s

[jkthorne]

This might not be a reasonible case but what happens if these are combined?

class Foo
  def baz
  end
end

class Bar < Foo
  def baz
  end

  @[Overwrite]
  def baz
  end

  @[Redefine]
  def baz
  end
end

It could happen with open classes or something but I dont know if it is valid to check against.

[akadusei]

  @[Overwrite]
  def baz
  end

My understanding is that @[Overwrite] was renamed to @[Redefine], so that's probably not valid. Maybe you mean @[Override] instead, thus?:

# ...

  @[Override]
  def baz
  end

  @[Redefine]
  def baz
  end

#...

It could happen with open classes or something but I dont know if it is valid to check against.

That should probably depend on the order they're added?

[asterite]

I see no problem with the above code.

[ysbaddaden]

If @[Redefine] will allow to have documented abstract methods that can be implemented elsewhere, yet keep their documentation, then it's a strong 👍 for me.

I'd love to refactor Crystal::System to implement abstract core/stdlib methods, versus oneliner method delegations, virtual types and more methods that bloat the source, the AST, and that LLVM must inline.

[vladfaust]

Is this going to be merged? I have a use-case of an advanced functionality shard which rewrites a function from a lesser functionality shard, and I have to make sure the former redefines the latter, i.e. there is a proper requiring order.

[asterite]

I'd avoid adding features at this point without a proper design phase.

[vladfaust]

Would such a design phase ever happen?

[vladfaust]

Looks like I have a super-power of cancelling features just by saying that I need them 😅

[j8r]

Does having this type of features behind a flag can be an option? This will bring user feedbacks, and the work won't be lost in the deep closed PR abyss.

[asterite]

@j8r I don't want to push a feature without a proper design phase, it doesn't matter if it's behind a feature flag. I pushed many features and stuff in the language without thoroughly thinking it and now I regret it (for example JSON::Serializable and user-defined attributes). Now it's time to rethink the language, keep what works and remove the excess (in my opinion).

[j8r]

@asterite don't regret what you have done.
It's not easy to know in advance what will work for the users or not. Without experimenting, we won't know at all :)

The only thing is it would be better in the future to have edge features behind flags (like annotations, too late), to ease the modification/removal of them like you said, without breaking the "stable" API.

[LeandroRezendeCoutinho]

Hello guys, i'm just a fan of crystal, but i have a suggestion.
I don't know how difficult it could be.
Why not have this feature as optional?
When used it must be checked, when not used, work like is today.

[straight-shoota]

@LeandroRezendeCoutinho As far as I know, this feature would only be optional. It just hasn't been implemented yet because we need properly designed semantics first. That design process hasn't started.