Crystal: Can't call re-defined method.

[MakeNowJust]

http://play.crystal-lang.org/#/r/9ov

def a
  p 1
end

a #=> 1

def a
  p 2
end

a #=> 1??

I think second a is wrong.

In addition, there work fine.

http://play.crystal-lang.org/#/r/9ow

def a
  p 1
end

# no call first a

def a
  p 2
end

a #=> 2

http://play.crystal-lang.org/#/r/9ox

def a
  p 1
end

a #=> 1

def a
  previous_def
  p 2
end

a
#=> 1
#=> 2

Why?

[asterite]

Yes, this is a known issue. Method redefinition is meant to be done before the "main" code is run. Once you invoke a method, that method is bound to that call and any subsequent calls, and redefining it has no effect.

We could still make it work somehow, but that would mean invalidating some cached information when you redefine a method. I'll mark this as an enhancement.

[asterite]

@MakeNowJust Why do you need this behaviour?

[MakeNowJust]

@asterite For example, I want to call second previous method.

http://play.crystal-lang.org/#/r/9oy

def foo
  p 1
end

$foo1 = ->foo

def foo
  previous_def
  p 2
end

foo
#=> 1
#=> 2


def foo
  $foo1.call
  p 3
end

foo
# expected
#=> 1
#=> 3

I think it is strange to change this behavior if redefined method has previous_def or not, because we are conscious of previous_def when we define redefined method.

[asterite]

@MakeNowJust After a few similar bug reports, I tried to see how difficult it would be to implement this. It turned out to be really easy to do, here's a patch for that. So if you apply that patch to the compiler, compile a new compiler and try your sample program, it prints "1213" :-)

The best thing is that all specs pass.

We _could_ just commit this, but I think it needs more thought. For example, right now if you have a method foo (argless) that returns an Int32, in the generated llvm the method is named *foo::Int32. This name is important because it's used in stack traces and will probably be used for debugging.

If we have many definitions for foo with the same return type, we can't give all of them the same name, because function names are unique. In the case of previous_def, we put a ' in the old method, so we get *foo':Int32. This is working right now, but I'm thinking this already makes it hard to know where foo' is defined.

Now we have foo being redefined without a previous_def but we want to remember it because it was used. So... the first quick solution I found was to use the object_id of the def in the function name. Ugly, but works.

Maybe these names don't matter much because this kind of redefinition won't be very common (at least regular non-redefined methods are much more common).

Another issue to think: you have $foo1 = ->foo. If I later redefine foo, what is the obvious thing to expect? Does $foo points to the old foo or to the new one? Do I want users to be able to redefine foo and keep a reference the old one, or to possibly new ones?

[asterite]

@MakeNowJust See for example https://github.com/manastech/crystal/issues/123 , here @kostya wants to keep a reference to a method but he wants users to be able to redefine that reference. So you want one behaviour, @kostya wants the opposite.

[ozra]

I know this issue is closed - but I find this important:

The most consistent behaviour in my eyes is that the last definition _always_ rule.
If you redefine a method - it's likely because you _want_ the changes to apply "upstream". Otherwise you'd define a unique method.

I would definitely expect the first example to print "2" twice.

[asterite]

I'm closing this. The way the compiler works now, and will work like this forever, is:

In the few first passes all types and methods are defined. The last defined method wins.

This means that this is the output of this program:

def a
  p 1
end

a # => 2

# This is the definition that wins for the whole program
def a
  p 2
end

a # => 2

In fact, since a couple of versions now you can invoke a method before defining it:

foo # works!

def foo
  puts "Hello!"
end

If you want to somehow reuse a previous definition you can use previous_def. That's all there is.

[vladfaust]

I think it's a right place...

module AA
  def foo
    puts "kek"
  end

  macro add_foo
    def foo
      previous_def
      {{yield}}
    end
  end
end

struct DA
  include AA

  add_foo do
    puts "lol"
  end
end

DA.new.foo

It's expected to output both "kek" and "lol", but there is no previous definition of 'foo'. Is that a bug?

[bew]

I suggest you to open a new issue.
I think it's different as your example involves macros as well.

[asterite]

previous_def finds in the same type. super finds in parents. You must use super here.

[vladfaust]

@asterite it doesn't work if calling multiple add_foo macros (latter takes precedence)

[Sija]

@vladfaust defining foo calling super inside DA makes the problem go away, see https://carc.in/#/r/4246, you can do this via included macro too - https://carc.in/#/r/4247. Bit hacky approach but it works 🙄

[vladfaust]

Disclaimer: I never get help with short-history gitter, stackoverflow seems a little overhead for this particular issue (which is my personal issue); I hope someone would help me here. The code will be opensourced anyway.

I'm implementing a versatile Callbacks system. I want Action to include Callbacks and make infinite-level inheritance with intermediate callbacks work.

module Callbacks
  def before
    true
  end

  macro before(&block)
    def before
      if previous_def
        {{yield}}
      end
    end
  end

  def with_callbacks(&block)
    before && yield # I omitted around and after callbacks 
  end
end

abstract struct Action
  include Callbacks

  abstract def call

  def call_with_callbacks
    with_callbacks { call }
  end
end

abstract struct UsersAction < Action
  before do
    puts "It's UsersAction"
    true
  end
end

struct RegisterUserAction < UsersAction
  before do
    puts "It's RegisterUserAction"
    true
  end

  def call
    puts "#call from RegisterUserAction"
  end
end

RegisterUserAction.new.call_with_callbacks
# Expected: It's UsersAction, It's RegisterUserAction, #call from RegisterUserAction
# Got: there is no previous definition of 'before'

I'm sorry for disturbing you all... :disappointed:

@Sija it hasn't worked for the whole issue.

[asterite]

If you hadn't used macros and the fancy dsl you seem to be trying to create, you would already have a working program by now.

Just stop using macros and magic so much and program what you need.

[vladfaust]

@asterite I’m creating a shard which would make programming with Crystal even more joy. This callbacks issue is the final milestone. Check prism.

If you find something useless that doesn’t mean it’s useless for everyone...