Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

super and previous_def ignore block arguments #10399

Open
HertzDevil opened this issue Feb 15, 2021 · 7 comments
Open

super and previous_def ignore block arguments #10399

HertzDevil opened this issue Feb 15, 2021 · 7 comments
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic

Comments

@HertzDevil
Copy link
Contributor

The following:

class Foo
  def foo
    yield
  end
end

class Bar < Foo
  def foo
    super
    super { puts 2 }
    super { puts 3 }
  end
end

Bar.new.foo { puts 1 }

only prints 1, whereas the same code prints also 2 and 3 in Ruby. Passing the wrong arguments to Bar#foo shows that these two overloads are defined:

  • Bar#foo()
  • Foo#foo(&block)

On the other hand, Bar.new.foo produces an undefined method 'foo' for Object error on the super call without a block. Similar errors also happen for previous_def. The current workaround is to declare a block and forward it manually, but now any other method parameters must also be manually forwarded in super:

class Bar < Foo
  def foo(&block)
    super(&block)
# ...

Ideally we should detect that Bar#foo in fact accepts a block, but is this possible? Or should super require a block argument if and only if the enclosing def uses a block elsewhere? (Note that both cases imply the block must be captured.)

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Jul 31, 2021
robacarp added a commit to mosquito-cr/mosquito that referenced this issue Nov 25, 2022
@asterite
Copy link
Member

I don't think this is a bug? In the end there's one overload with a block and one without one.

In reality I think we might need to change the language so that if a method exists in a type then methods are looked up in just the type. If ko overload matches it's an error. Parent methods are only looked up if no method with that name exist in the child.

I'll later send examples with code. But with the current semantics this isn't a bug.

@beta-ziliani
Copy link
Member

I do consider this an error, as the actual behavior isn't consistent.

What I would expect is to Bar#foo be considered as not taking a block (as now), but then error because there's no Foo#foo() to call with super. I think this goes in line with what you're saying.

@Sija
Copy link
Contributor

Sija commented Nov 25, 2022

Since the first call to super in Bar#foo only forwards the block, shouldn't the whole method be defined as Bar#foo(&) only?

@beta-ziliani
Copy link
Member

Since the first call to super in Bar#foo only forwards the block

How do you know that? What if Bar defines a foo() method? To me, in such cases one needs to be explicit about the block.

@Sija
Copy link
Contributor

Sija commented Nov 25, 2022

I know that because the super call requires it. Also, I don't follow - Bar defines foo...

@straight-shoota
Copy link
Member

@beta

but then error because there's no Foo#foo() to call with super

Isn't that exactly what's happening right now?

class Foo
  def foo
    yield
  end
end

class Bar < Foo
  def foo
    super # Error: undefined method 'foo' for Object
  end
end

Bar.new.foo

@robacarp
Copy link
Contributor

I don't think I have a preference about the behavior but it would certainly help to improve the error message, though perhaps that's tracked somewhere else. Run this code on Crystal 1.6.2:

require "colorize"

class Foo
  def only_if_coordinator : Nil
    yield
  end
end

class Bar < Foo
  def only_if_coordinator : Nil
    if false
      yield
    else
      super
    end
  end
end

b = Bar.new

b.only_if_coordinator do
  pp "blah"
end

The error message is:

Error: undefined method 'only_if_coordinator' for Colorize::ObjectExtensions

Which doesn't provide any helpful guidance on why it can't find a method, or why it's looking at Colorize::ObjectExtensions. The source of that name is certainly some bubbling up behavior where it's looking for the appropriate super candidate, but it's pretty confusing.

icy-arctic-fox added a commit to icy-arctic-fox/spectator that referenced this issue Dec 18, 2022
The previous_def and super keywords do not propagate blocks.
See: crystal-lang/crystal#10399
This works around the issue by populating arguments if the method uses a block.
robacarp added a commit to mosquito-cr/mosquito that referenced this issue Sep 29, 2023
robacarp added a commit to mosquito-cr/mosquito that referenced this issue Sep 29, 2023
* implements a distributed lock for worker coordination

The distributed lock system is stable up until the point that the redis cluster needs multiple masters.

* refactors runner into logical separations

splits into coordinator/executor/queue_list

* simplifies naming and log messages re: delayed/periodic management

* flags run_cron_scheduler as deprecated

the usage has been removed completely

* provides overseer, run_at_most for runner refactor

* Move spin loop into overseer

Runner is now simply the api to affect the overseer, and it will be the place for multiple overseer threads to attach

* tests for coordinator

* tests for queuelist

* tests for overseers

* tests for executor

* tests for run_at_most

* tests for log messages from executor

* final testing cleanup for runner refactor

* refactor run_at_most for monotonic

* refactors Base.job_for_type to use String.build

* tests for coordinator locking

painfully working my way around crystal-lang/crystal#10399

* tests for backend lock methods

* tests for coordinator locking

* give a little grace to scheduled job test time constraint

* allows script failures due to database flush to be caught and rescued

* refactors lock_test to ensure locks are always unlocked

prevents intermittent test failures

* refactor: switch let statements to getter statements

* changelog entries

* disables distributed lock functionality by default, opt-in for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

No branches or pull requests

6 participants