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

Translate RBI attached_class to RBS instance #384

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Dec 10, 2024

The attached_class in RBI:

sig { returns(T.attached_class) }
def foo; end

should be translated to instance in RBS:

def foo: -> instance

@Morriar Morriar added the bugfix Fix a bug label Dec 10, 2024
@Morriar Morriar self-assigned this Dec 10, 2024
@Morriar Morriar requested a review from a team as a code owner December 10, 2024 21:02
@paracycle
Copy link
Member

Are you sure that that is the right translation? The example in the RBS docs doesn't make it sound like it is so:

class Foo
  attr_reader parent: class

  def foo: () -> instance

  @@foos: Array[instances]

  include Enumerable[class]
end

Note that the foo method is an instance method, so the concept AttachedClass does not even make sense.

Does instance mean a more general "attached object" then?

@Morriar
Copy link
Collaborator Author

Morriar commented Dec 11, 2024

I'm not sure about the foo method in this case, the class attribute would make sense (it seems there is a typo in the example though):

class Foo
  attr_reader parent: class
  @@foos: Array[instances]
end

The document says:

instance denotes the type of instance of the class. class is the singleton of the class.

Looking at how it is used in the RBS files for core it seems to mean T.attached_class?

The usage in foo is indeed surprising. I though that depending on the context it could mean T.self_type but there is already self in RBS. I'll inquire.

Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

LGTM, pending clarification of how instance is intended to work in RBS.

@Morriar
Copy link
Collaborator Author

Morriar commented Dec 17, 2024

I though that depending on the context it could mean T.self_type but there is already self in RBS. I'll inquire.

So it's indeed dependent on the context.

  • On a class method it does return the type of the instance, so the same as T.attached_class.
  • On an instance method it behaves as self, so it should be translated as T.self_type.

I think this is useful for RBS methods declared with the syntax def self?.foo: instance where the return type will be instance in the case of the singleton and self in the case of the instance.

Since we come from RBI, if we find a T.attached_class we can presume that it was used in the context of a singleton method (or Sorbet would have errored) so I think it's safe to translate it to instance.

@Morriar Morriar merged commit b1dd32b into main Dec 17, 2024
8 checks passed
@Morriar Morriar deleted the at-rbs-attached-class branch December 17, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants