Crystal: New keyword to re-open classes

[alex-lairan]

Nowadays, if I want to modify a class, I just have to redefine it.

class Foo
  def bar
    5
  end
end


class Foo
  def bar
    9
  end
end

The problem with that is, if I have multiples file, we don't know witch one will be used.
(This is a design problem, and we can blame the developer, but it's not the purpose of this example)

So, I propose this little change (with breaking change so ..), add a keyword to open classes and disallow class change without it.

class Foo
  def bar
    5
  end
end

open Foo
  def bar
    9
  end
end

The problem can still exists if we have multiples opens, but it will solve lot of them.

Another point for this is simplicity,

If I have a class like this :

module Animals
  abstract class Base
  end

  class Dog < Base
  end
end

I don't know if I should do

module Animals
  class Dog < Base
    def bark
      "bark"
    end
  end
end

or this

module Animals
  class Dog
    def bark
      "bark"
    end
  end
end

The keyword can be this :

open Animals::Dog do
  def bark
    "bark"
  end
end

Here a list of advantages

• Better lisibility
• Avoid override existing classes
• Simplify the overriding concept
• An information for incremental compilation (I really love this one)

[jhass]

Not sure I fully get the issues this is trying to solve, but I don't feel like this is necessary.

In any case I feel like this is gonna duplicate a lot of the discussion that happens in #1647, so it might be worth to wait until we have a conclusion there before progressing too much into this one.

[alex-lairan]

The two ideas are not the same.

Here, it's to simplify incremental compilation, and bonus, help people to do class opening, without breaking everything

[asterite]

Many times I wanted something like this myself. Maybe use reopen as a keyword.

I think the idea is to make sure that you are reopening a class and not defining it for the first time.

Sometimes it happens that, because of how things are required, a file gets required before another file where the original intention was to require things in another order. If that happens some of these things will happen:

• if a type was to be reopened you usually leave out the parent type, like class Dog, and later on you get an error on class Dog < Animal saying "superclass mismatch: Animal vs. Reference", which is confusing
• if the parent class is implicitly Reference, or you specify it in both places, you might silently get an overload wrong

Having a reopen type is also good to avoid specifying what a type is: a class, struct or module. You can change the main type and reopen will continue to work just fine.

I like the idea but I'd like to know what others think about it. Introducing a new keyword is always a breaking change if someone was using it for something else.

[asterite]

With reopen we can also let the doc tool ignore comments on it.

[jhass]

We're very far from supporting incremental compilation to be honest, so this feels like premature optimization at least to me.

Besides, incremental compilation will still probably mean one object file per type and then being smart about detecting changes to that type. This means the compiler will still have to combine all the code touching the definition of the type first. I don't see how a different keyword makes any difference here, we could easily imagine a language where each change to a type, like adding a method or adding a field requires "opening the type" again. It's all just syntactic sugar. Maybe I'm missing something...

I reference #1647 because a lot of the discussion there is using a keyword vs an annotation, which applies here as well.

[alex-lairan]

We're very far from supporting incremental compilation

So let's move step by step :)

and then being smart about detecting changes to that type.

But this may be not possible. How do you know the one who is the base, and who is the extension.

Also, If I have a simple class like :

class Foo
end

And someone do

class Foo < Bar
  def ...
  end

If we base on the complexity, the 2nd is the base
If we base on the definition order, the 1st is the base, and will break the second one.

@asterite reopen seems great ! I like it !

[jhass]

Why do I need to know the base? Why do I even need the concept of a base?

[alex-lairan]

Because of inheritance

[jhass]

I don't understand, please expand. Let's keep the terms "base" as in "base class of a hierarchy" and as in "base definition of a type with some reopenings" differentiated here.

[straight-shoota]

So let's move step by step :)

Sorry, that won't work. We need to lay out a path first. It doesn't make sense to make a pace forward if we only guess it might be in the right direction.
"helps incremental compilation" is not a valid argument for such a change until there is proof that it really does.

[asterite]

I mainly see this as a way to make it clearer that a definition of a type is meant to be a reopening of an existing type. And I think that's valuable.

[jhass]

I mainly see this as a way to make it clearer that a definition of a type is meant to be a reopening of an existing type. And I think that's valuable.

I can see that but I would prefer that as an optional annotation over a new keyword. Something I can ask the compiler to check for me rather than it forcing me to think about it.

[vlazar]

Maybe refine keyword instead of reopen? Ruby has refine which is not a keyword, but it's not widely used. No a lot room for confusion.

[asterite]

I can see that but I would prefer that as an optional annotation over a new keyword

That doesn't have the advantage of not caring what the reopened type is.

If I want to add a method to Int32 I might go and do:

class Int32
  # whatever
end

and the compiler will complain saying "Int32 is not a class, it's a struct".

But I don't care what type is the thing I'm reopening, I only care about adding stuff to it. And an annotation can't solve that.

[asterite]

One more thing: let's say I want to reopen String and add a method to it:

class Stirng
  def this_method_is_awesome!
    # It really is!
  end
end

But can you spot the bug?

[jhass]

But I don't care what type is the thing I'm reopening, I only care about adding stuff to it.

Is that so? Especially when adding more complex things, whether I'm working inside a reference or value type can be quite relevant information. What if I am reopening a library's type and the library changes from class to struct? Updating my patch might seem tedious but I think it's actually valuable to have to consider whether the change impacts it.

But can you spot the bug?

Well I can and the annotation would help as well here.

[bcardiff]

I agree with @jhass it feels similar to #1647 and I don't grasp how it will help incremental compilation. It is definitely a way to express better the intention of the programmer and it will catch renamings issues, typos and can be used for doc tools.

But I am hesitant to make it mandatory to annotate as @[Reopen] if there is an existing definition of the type.

I think is also better to keep the explicit class, struct, module mention in the reopen, because things can change and it will more difficult to spot. Similar to the recent change of repeating the return type of abstract methods.

[wontruefree]

I like having open classes by default it enables frameworks/debugging/general extension.

What about adding a hook when a class is reopened? Using this pattern https://crystal-lang.org/reference/syntax_and_semantics/macros/hooks.html

As a base class if you want to lock down your class you can require block.

class String
  reopen_protection true
end

# WORKS
class String
  reopen do
    def awesome_method!
      # something awesome
    end
  end
end

# COMPILE ERROR
class String
  def awesome_method!
    # something awesome
  end
end

[straight-shoota]

I don't think that makes any sense to introduce two different behaviours. The distinction would be rather arbitrary.

[wontruefree]

I don't have a problem with open classes, I only bring up the option because people consistently have concerns about open classes and having the option to lock down a class seems like a wanted feature.

I think Crystal also prioritizes expected behavior over correctness when appropriate and I think having locked down classes in general breaks that expectation.

I think in general reopening classes is a problem both Ruby and Crystal struggle with. I don't really know how to map this to the goals of Crystal.

[asterite]

I think reopening classes in Crystal is fine and less problematic than in Ruby. The problem with Ruby is that you get the failure at runtime and it's not clear why it happens. Having two shards reopen a same class with a different implementation or behavior will probably mean a compile-time error and it should be easier to fix, though this is not guaranteed.

I don't think we should work against the features provided by the language. Otherwise I would suggest we think how we can change Crystal to remove open classes.

[Blacksmoke16]

Isn't this similar to what final would do?

final def foo # => Compile error if you try to redefine it.
final class Bar # => Compile error if you try to inherit/reopen it

This would at least preserve the ability to reopen classes, but provide the option to the developer to make it so it can't be done.

[straight-shoota]

@Blacksmoke16 That's a related topic. But it's still a different story whether I want to close a class/def or regulate redefinition/inheritance to take extra care.

[straight-shoota]

Btw. inheriting can be forbidden by macro inherited; {% raise "closed type" %};end. Maybe there could be macro hook reopened which could be used in a similar way?
However, I can't think of any other, natural use case for this, so it probably isn't a good idea.

[asterite]

I'd like to forbid redefining the question methods generated for enum members, same for Enum#=== and Class#=== so that exhaustiveness checks are guaranteed by the compiler and can't be broken by a user redefining them. But that's the only use case I have in mind so far. And I'd use an annotation on top of a method for that.

[straight-shoota]

@asterite Since it needs to be known by the compiler anyway, you don't necessarily even need an annotation for that. Could just implement it as a special compiler feature.

[RX14]

If you leave the original class definition out, you'll get a compile error anyway. So this change is intended to help in the case of compile-order problems, and that's cool!

But, that means that this will help when redefining or monkey patching methods inside the existing class, not just when having a class defined over multiple files (like say Program or all the different AST nodes in the compiler).

So, to me, this sounds more like a problem which a "redefines" or "overrides" annotation will solve more comprehensively.

[ryanprior]

A pattern I often use in Ruby & don't know how to replicate in Crystal is to refine a class like so:

module Emphasis
  refine String do
    def to_emphatic
      "#{self}!!"
    end
  end
end

module Main
  using Emphasis
  def self.run
    puts "Hello world".to_emphatic
  end
end

p "Hello world".respond_to? :to_emphatic # => false
Main::run # => "Hello world!!"

I love this pattern because it lets me add to base classes with wild abandon without risking that I will mess up behaviors in somebody else's module. Is there already a way to get that kind of hygenic class refinement in Crystal? Or is this something we could think about adding, in the vein of this thread?

[bcardiff]

@ryanprior no, there is not. A related discussion happened in #6438.

I personally have not used refinements in ruby so it's hard to jump into that. I see the value though, but if the operation does not need to deal with internal representation, then I usually turn into helper functions.

Top-level private methods or modules are scoped by file. That is something that can help in this scenario to some extent.

[vlazar]

Ruby refinements seem to be such a nice feature, except they haven't became popular for some reason. Rails for example has only 1 usage https://github.com/rails/rails/search?q=refine&unscoped_q=refine

[RX14]

@ryanprior the idiomatic crystal would be just

module Main
  def self.run
    puts emphasize("Hello world")
  end

  private def self.emphasize(str)
    "#{str}!!"
  end
end

Main::run # => "Hello world!!"

Especially since you don't need access to String's internals for this.

[ryanprior]

@RX14 thank you for confirming! That's similar to how I've been writing my code lately. What I miss about the way I write Ruby code using refinements is how it interacts with Ruby/Crystal's built-in assumptions about where methods belong, eg it's shorter to write

statements.map(&.to_emphatic)

...than to write

statements.map { |statement| emphasize(statement) }

Since the Crystal language rewards you with sweet syntax for putting methods on the classes they act upon, refinements (or a similar mechanism) would help me express my program logic in a way that utilizes those nice features without clobbering those types for everybody else.

[RX14]

I've always thought that crystal needed a syntax for this, map(&->emphasize(String)) works right now, but it would be nice to at least remove the performance hit of going to proc and back, and ideally not have to specify argument types.