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

Treat present? as a special method that guarantees the receiver is not nil #1130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mtsmfm
Copy link

@mtsmfm mtsmfm commented May 20, 2024

Fixes #472

The pull request #905 attempts to resolve the issue using annotations, but this PR resolves it by treating present? as a special method, similar to how Sorbet handles it https://github.com/sorbet/sorbet/blob/718bc64f895abeeff88f56efbc92a3554ffaa4f2/infer/environment.cc#L564-L579

Comment on lines +303 to +325
when AST::Types::Logic::ReceiverIsNotNil
if receiver && arguments.size.zero?
receiver_type = typing.type_of(node: receiver)
unwrap = factory.unwrap_optional(receiver_type)
truthy_receiver = unwrap || receiver_type
falsy_receiver = AST::Builtin.nil_type

truthy_env, falsy_env = refine_node_type(
env: env,
node: receiver,
truthy_type: truthy_receiver,
falsy_type: falsy_receiver
)

truthy_result = Result.new(type: TRUE, env: truthy_env, unreachable: false)
truthy_result.unreachable! unless unwrap

falsy_result = Result.new(type: FALSE, env: falsy_env, unreachable: false)
falsy_result.unreachable! if no_subtyping?(sub_type: AST::Builtin.nil_type, super_type: receiver_type)

[truthy_result, falsy_result]
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part would be the exactly the same as https://github.com/soutaro/steep/pull/905/files#diff-56dab2e481abd1ca9efdcfd898234599b5f7ad222e557cf242db1d3c6904516a

@tk0miya Could you extract this part as different PR?

In either way we need this patch.

@soutaro
Copy link
Owner

soutaro commented Jun 7, 2024

Thank you for opening this PR. I think there are two things to consider:

  1. It's not ReceiverIsNotNil. Introducing ReceiverIsPresent or something would be better. (And because the negation doesn't mean it's nil, the implementation should be revised.)
  2. I'm not very sure if we should have this implementation in Steep. It's not a part of Ruby. The semantics look like too complicated.

@ksss
Copy link
Contributor

ksss commented Jun 7, 2024

Do we need steep-rails?

@soutaro
Copy link
Owner

soutaro commented Jun 7, 2024

We can support these non-Ruby methods by extending RBS types where we can directly write something like:

  def present?: () -> self is not blank

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

Successfully merging this pull request may close these issues.

Steep doesn't handle Object#present? correctly
3 participants