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

Partial support for is_a? with exhaustiveness #781

Open
jez opened this issue Jun 8, 2019 · 0 comments · Fixed by #4373
Open

Partial support for is_a? with exhaustiveness #781

jez opened this issue Jun 8, 2019 · 0 comments · Fixed by #4373
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jez
Copy link
Collaborator

jez commented Jun 8, 2019

Input

→ View on sorbet.run

# typed: true

class A
  extend T::Sig
  sig {params(foo: T.any(Integer, String)).returns(Integer)}
  def bar(foo)
    if foo.is_a?(Integer)
      foo
    elsif foo.is_a?(String)
      foo.to_i
    end
  end
end

Observed output

editor.rb:6: Returning value that does not conform to method result type https://sorbet.org/docs/error-reference#7005
     6 |  def bar(foo)
     7 |    if foo.is_a?(Integer)
     8 |      foo
     9 |    elsif foo.is_a?(String)
    10 |      foo.to_i
    11 |    end
    12 |  end
  Expected Integer
    editor.rb:6: Method bar has return type Integer
     6 |  def bar(foo)
          ^^^^^^^^^^^^
  Got T.nilable(Integer) originating from:
    editor.rb:6:
     6 |  def bar(foo)
          ^^^^^^^^^^^^
    editor.rb:6:
     6 |  def bar(foo)
                  ^^^
    editor.rb:7:
     7 |    if foo.is_a?(Integer)
               ^^^^^^^^^^^^^^^^^^
    editor.rb:10:
    10 |      foo.to_i
              ^^^^^^^^
Errors: 1

Expected behavior

Since we've exhausted all the cases, it should be fine to return from this method (the inferred method return type should not include NilClass.

This can be solved the same way as #59 (see #743).

@jez jez added bug Something isn't working unconfirmed This issue has not yet been confirmed by the Sorbet team good first issue Good for newcomers and removed unconfirmed This issue has not yet been confirmed by the Sorbet team labels Jun 8, 2019
@jez jez added this to To do in ruby-lang-team via automation Jul 13, 2021
jez added a commit that referenced this issue Jul 16, 2021
I took a stab at trying to fix #4358 but stalled out on it.

The approach I took was to attempt to just use whether the
flow-sensitive call was on a `<cfgAlias>` variable (only created by
`cfg::Alias` instructions).

The problems I ran into were:

- Using only names wasn't powerful enough to see through static-fields.
  The only way to properly handle static fields would be to maintain a
  mapping of whether the alias that initialized a `<cfgAlias>` was
  actually a static-field symbol or not.

- I would have been fine with that, because it would have made some of
  the bugs go away, just not all of them.

  The next problem was that changing *only* `updateKnowledge` was not
  enough. For any of these methods (`Kernel#is_a?`, `Module#===`,
  `Module#<=`), if they have an intrinsic that computes the result type
  to be `TrueClass` or `FalseClass` exactly (not `T::Boolean`), that
  also introduces the same dead code error.

  So to fix the dead code error, you need to fix **both** the case in
  `updateKnowledge` and the intrinsic for that method.

  At the time of writing, only `Module#===` has such an intrinsic, but
  honestly, I'd rather **add** intrinsics for `is_a?` / `<=` (to fix
  this bug: #781) than I would
  like to figure out how to fix the existing ones.

  Basically: adding a halfway-working solution here would make it harder
  to fix #781 in the future, and
  I don't want that.
@jez jez closed this as completed in #4373 Jul 16, 2021
ruby-lang-team automation moved this from To do to Done Jul 16, 2021
jez added a commit that referenced this issue Jul 16, 2021
I took a stab at trying to fix #4358 but stalled out on it.

The approach I took was to attempt to just use whether the
flow-sensitive call was on a `<cfgAlias>` variable (only created by
`cfg::Alias` instructions).

The problems I ran into were:

- Using only names wasn't powerful enough to see through static-fields.
  The only way to properly handle static fields would be to maintain a
  mapping of whether the alias that initialized a `<cfgAlias>` was
  actually a static-field symbol or not.

- I would have been fine with that, because it would have made some of
  the bugs go away, just not all of them.

  The next problem was that changing *only* `updateKnowledge` was not
  enough. For any of these methods (`Kernel#is_a?`, `Module#===`,
  `Module#<=`), if they have an intrinsic that computes the result type
  to be `TrueClass` or `FalseClass` exactly (not `T::Boolean`), that
  also introduces the same dead code error.

  So to fix the dead code error, you need to fix **both** the case in
  `updateKnowledge` and the intrinsic for that method.

  At the time of writing, only `Module#===` has such an intrinsic, but
  honestly, I'd rather **add** intrinsics for `is_a?` / `<=` (to fix
  this bug: #781) than I would
  like to figure out how to fix the existing ones.

  Basically: adding a halfway-working solution here would make it harder
  to fix #781 in the future, and
  I don't want that.
@jez jez reopened this Jul 16, 2021
ruby-lang-team automation moved this from Done to In progress Jul 16, 2021
@jez jez moved this from In progress to To do in ruby-lang-team Jul 16, 2021
@jez jez removed this from To do in ruby-lang-team Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant