static: Covariance rule is not checked for `attr_reader`

Morriar commented 5 years ago

Input

See classes Error1, Error2, and Error3.

→ View on sorbet.run%7D%20%23%20WRONG%3A%20Should%20error%2C%20as%20nilable%20Object%20is%20not%20covariant%20with%20A%0A%20%20attr_reader%20%3Aa%0A%0A%20%20sig%20%7B%20void%20%7D%0A%20%20def%20initialize%0A%20%20%20%20%40a%20%3D%20T.let(nil%2C%20T.nilable(Object))%0A%20%20end%0Aend%0A%0Aclass%20Error3%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(T.nilable(B))%7D%20%23%20WRONG%3A%20Should%20error%2C%20as%20nilable%20B%20is%20not%20covariant%20with%20A%0A%20%20attr_reader%20%3Aa%0A%0A%20%20sig%20%7B%20void%20%7D%0A%20%20def%20initialize%0A%20%20%20%20%40a%20%3D%20T.let(nil%2C%20T.nilable(B))%0A%20%20end%0Aend%0A%0A%23%20The%20following%20tests%20should%20still%20work%20once%20fixed%3A%0A%0Aclass%20Ok1%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(B)%7D%20%23%20Ok%2C%20covariance%0A%20%20attr_reader%20%3Aa%0A%0A%20%20sig%20%7B%20params(b%3A%20B).void%20%7D%0A%20%20def%20initialize(b)%0A%20%20%20%20%40a%20%3D%20T.let(b%2C%20B)%0A%20%20end%0Aend%0A%0Aclass%20Ok2%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(B)%7D%20%23%20Ok%0A%20%20def%20a%0A%20%20%20%20B.new%0A%20%20end%0Aend%0A%0A%23%20This%20already%20work%20well%20with%20concrete%20methods%3A%0A%0Aclass%20Ok3%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(T.nilable(B))%7D%20%23%20Ok%3A%20errors%20as%20it%20should%0A%20%20def%20a%0A%20%20%20%20B.new%0A%20%20end%0Aend%0A%0Aclass%20Ok4%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(Object)%7D%20%23%20Ok%3A%20errors%20as%20it%20should%0A%20%20def%20a%0A%20%20%20%20B.new%0A%20%20end%0Aend%0A%0Aclass%20Ok5%20%3C%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20override.returns(Unrelated)%7D%20%23%20Ok%3A%20errors%20as%20it%20should%0A%20%20def%20a%0A%20%20%20%20Unrelated.new%0A%20%20end%0Aend)

# typed: strong

class A; end
class B < A; end

class Unrelated; end

class Foo
  extend T::Sig
  extend T::Helpers

  abstract!

  sig { abstract.returns(A)}
  def a; end
end

# Covariance rule for attr readers return types is not checked:

class Error1 < Foo
  extend T::Sig

  sig { override.returns(Unrelated)} # WRONG: Should error, as Unrelated is not a subtype of A
  attr_reader :a

  sig { params(a: Unrelated).void }
  def initialize(a)
    @a = T.let(a, Unrelated)
  end
end

class Error2 < Foo
  extend T::Sig

  sig { override.returns(T.nilable(Object))} # WRONG: Should error, as nilable Object is not covariant with A
  attr_reader :a

  sig { void }
  def initialize
    @a = T.let(nil, T.nilable(Object))
  end
end

class Error3 < Foo
  extend T::Sig

  sig { override.returns(T.nilable(B))} # WRONG: Should error, as nilable B is not covariant with A
  attr_reader :a

  sig { void }
  def initialize
    @a = T.let(nil, T.nilable(B))
  end
end

# The following tests should still work once fixed:

class Ok1 < Foo
  extend T::Sig

  sig { override.returns(B)} # Ok, covariance
  attr_reader :a

  sig { params(b: B).void }
  def initialize(b)
    @a = T.let(b, B)
  end
end

class Ok2 < Foo
  extend T::Sig

  sig { override.returns(B)} # Ok
  def a
    B.new
  end
end

# This already work well with concrete methods:

class Ok3 < Foo
  extend T::Sig

  sig { override.returns(T.nilable(B))} # Ok: errors as it should
  def a
    B.new
  end
end

class Ok4 < Foo
  extend T::Sig

  sig { override.returns(Object)} # Ok: errors as it should
  def a
    B.new
  end
end

class Ok5 < Foo
  extend T::Sig

  sig { override.returns(Unrelated)} # Ok: errors as it should
  def a
    Unrelated.new
  end
end

Observed output

Only the error from classes OkX are found by the static checker:

editor.rb:85: Return type T.nilable(B) does not match return type of abstract method Foo#a https://srb.help/5035
    85 |  def a
          ^^^^^
    editor.rb:15: Super method defined here with return type A
    15 |  def a; end
          ^^^^^

editor.rb:94: Return type Object does not match return type of abstract method Foo#a https://srb.help/5035
    94 |  def a
          ^^^^^
    editor.rb:15: Super method defined here with return type A
    15 |  def a; end
          ^^^^^

editor.rb:103: Return type Unrelated does not match return type of abstract method Foo#a https://srb.help/5035
     103 |  def a
            ^^^^^
    editor.rb:15: Super method defined here with return type A
    15 |  def a; end
          ^^^^^
Errors: 3

Expected behavior

I would expect Sorbet to catch the bad redefinitions for the return type of attr_reader :a in Error 1, 2, and 3.

This works correctly with the runtime checker.

jez commented 5 years ago

There are 3 places in definition_validator where we check whether a method was defined in a DSL, and skip some checks if so:

https://github.com/sorbet/sorbet/blob/586a106954f17f27655455dcf6580424583d6b1e/definition_validator/validator.cc#L316-L332

The reasoning for this IIRC was because it made some things easier to roll out internally, and also because for some DSL-synthesized methods (like T::Struct props) there's no syntax to specify override when it's required (because a parent method had abstract).

Unfortunately that skips out on a lot of good checks that we should probably be doing.

jez commented 5 years ago

@aisamanra can you chime in? do you have more context than I do here?

sorbet / sorbet