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

initialize now considered private in interface #6804

Closed
paddyobrien opened this issue Mar 2, 2023 · 3 comments
Closed

initialize now considered private in interface #6804

paddyobrien opened this issue Mar 2, 2023 · 3 comments

Comments

@paddyobrien
Copy link
Contributor

Input

This appears to be a change in semantics from 0.10.5 and 0.11.1

→ View on sorbet.run

# typed: true
module MyInterface
  extend T::Sig
  extend T::Helpers
  interface!

  sig do
    abstract.params(
      foo: T.nilable(String)
    ).void
  end

  def initialize(foo); end
end

Observed output

editor.rb:13: Interface method MyInterface#initialize cannot be private https://srb.help/5042
    13 |  def initialize(foo); end
          ^^^^^^^^^^^^^^^^^^^
Errors: 1

Expected behavior

This did not cause an error in 0.10.5 but does in 0.11.1. Not sure if this is an intentional change?


@jez
Copy link
Collaborator

jez commented Mar 2, 2023

I have no clue what those versions are—they are not Sorbet version numbers.

This is sort of an expected change, but also an unexpected change. This is the real bug: #5687, it's just that after a recent change, this bug gets tripped under the covers.

But I'm also surprised to hear that this was ever allowed: when I run that program, I get an exception:

# typed: true
require 'sorbet-runtime'

module MyInterface
  extend T::Sig
  extend T::Helpers
  interface!

  sig do
    abstract.params(
      foo: T.nilable(String)
    ).void
  end

  def initialize(foo); end
end

class A
  extend T::Sig
  include MyInterface
  sig do
    override.params(
      foo: T.nilable(String)
    ).void
  end

  def initialize(foo); end
end

A.new('')
❯ ruby foo.rb
Traceback (most recent call last):
        8: from foo.rb:30:in `<main>'
        7: from foo.rb:30:in `new'
        6: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:256:in `block in _on_method_added'
        5: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:425:in `maybe_run_sig_block_for_key'
        4: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:445:in `run_sig_block_for_key'
        3: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:246:in `block in _on_method_added'
        2: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:344:in `run_sig'
        1: from /home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/_methods.rb:388:in `build_sig'
/home/jez/.rbenv/versions/2.7.7/lib/ruby/gems/2.7.0/gems/sorbet-runtime-0.5.10666/lib/types/private/methods/signature_validation.rb:14:in `validate': `initialize` should not use `.abstract` or `.implementation` or any other inheritance modifiers. (RuntimeError)

Fixing this behavior is tracked by this issue: #3388.

So I think that while it's clear that a recent change caused this new error to appear, at runtime, that code was never allowed anyways, and as such we should simply roll forward by fixing the two mentioned bugs, instead of rolling back the PR that caused this.

@paddyobrien
Copy link
Contributor Author

Apologies, the correct version numbers are 0.5.10648 to 0.5.10693

(the listed ones were tapioca versions)

So I think that while it's clear that a recent change caused this new error to appear, at runtime, that code was never allowed anyways, and as such we should simply roll forward by fixing the two mentioned bugs, instead of rolling back the PR that caused this.

I haven't had any problems at runtime with this code, but I agree that fixing the root cause makes sense rather than a roll back.

I note both those issues are marked as "good first issue" so I'll see if I can make any progress on them!

@jez
Copy link
Collaborator

jez commented Apr 1, 2023

Closing in favor of #3388 and #5687

@jez jez closed this as not planned Won't fix, can't repro, duplicate, stale Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants