soutaro / rbs-inline

Inline RBS type declaration
MIT License
231 stars 7 forks source link

rbs syntax only #9

Closed HoneyryderChuck closed 3 months ago

HoneyryderChuck commented 5 months ago

First of all, thx for working on this! Inline sigs transpiling to rbs files is one of the solutions which crossed my mind at some point in order to solve the IMO loudest negative feedback towards rbs, which is having signatures not co-located with source code (The other was showing and remote-editing sigs on hover as an LSP-based feature, which would only work with IDEs, and your proposal does not have to trade off on that).

Here's my feedback:

Support RBS syntax only

The current proposal presents syntax which is somewhat inspired by yard ("@rbs" followed by keyword for param, returns, yields...). Preferences aside, I think that RBS should not support "yet another" syntax, and just stick to the main RBS syntax. As a steep user, it's already mentally taxing to keep track of the different ways one can annotate variables inline (# @type var other: Phone), and while that's not rbs syntax, it'd nonetheless leak to the (AFAIK) only static type checker supporting RBS atm, i.e. there'd be 3 ways to type things (although I guess this already proposes a way to replace steep inline type annotations?).

I'd stick to RBS syntax because it's obviously more flexible and powerful than yard-style annotations, which would have a hard time supporting overloading, complex block definitions or generics.

Perhaps the idea is to ease migration from projects already using yard, but IMO this only makes it more complex, both to use and maintain, and the problem of yard has already been solved (sord can already translate yard to rbs).

  # @rbs (name:String, addresses: Array[String]) -> void
  def initialize(name:, addresses:)
    @name = name
    @addresses = addresses
  end

👍 abbreviations

I like that one can define return types inline, although I'd suggest using -> instead:

def to_s # -> String
   "bang"
end

# works also with endless methods?
def to_s = "bang" # -> String

Consider IDE integration

One obvious way this could integrate in IDEs is to transpile on save. The current implementation seems to be optimized for running as a script, so one think to keep track of going forward is to make sure it translates well in a "listen to file event" scenario (tbh this is smth I'd also would like steep to do, since running steep check loads the rainbow multi-thread stack, and I can't easily use a debugger to investigate rbs/steep issues).

monkeypatch bundler

The target audience seems to be application users right now. However, consider also this syntax for gem maintainers, which would benefit from deeper integration with rubygems/bundler when it came to declaring where to find sig files, or "transpile on packaging". I think that, in particular in the cases of older gems which haven't been adopting rbs, this quasi-doc syntax would be much more welcomed, as long as the operational burden would remain low.

This could be achieved now via some monkeypatching, which could later be upstreamed to rubygems/bundler, who have been mostly neutral about better type sig support in the overall rubygem structure (there isn't yet a way to declare the sig files for gems, just like one can do for lib and test files).

ParadoxV5 commented 5 months ago

Porting my comments from the #rbs channel on Discord over here…

Support RBS syntax only

I prefer this too and for the same reasons.

Designed to be a stand-alone document, RBS itself already has the constructs to annotate (almost) everything without needing directive commands. Correspondingly, RBS::Inline do not need to have all those directives such as yields and returns. The full doc-style # @rbs should maintain a consistent identifier: type format, as if type checkers like Steep can repurpose the syntax for local variables. [P.S. No more the need to escape conflicting arg names.]

Instead of:

# @rbs yields (String) -> void
# @rbs returns void
def each_address(&block)

We can just have:

# Note: A hypothetical Ruby 4 could deprecate `yield` in favor or `blk.call`
# @rbs block: ^{ (String) -> void } # Type of block arg must be a proc
# @rbs return: void # `return` is not a valid identifier for a Ruby arg
def each_address(&block)

Moreöver, instead of:

# @rbs args: Array[String]
# @rbs kwargs: Hash[Symbol, untyped]
# @rbs yields (String) -> void
def example(*args, **kwargs, &block)

(Must be Hash[K, V]? Non-Symbol Hash isn’t supported in RBS itself: ruby/rbs#1645) We could:

# @rbs *: String
# @rbs **: untyped
# @rbs &: (String) -> void
def example(*args, **kwargs, &block)

What to do with type args (generics) becomes a concern, though, and not just for superclasses but also supermodules [EDIT: and self bounds (for Modules) too].

# @rbs generic unchecked in V < String
# @rbs inherits Hash[Integer, V]
class Foo < Hash

Writing the RBS in full directly would could be

#:: Foo[unchecked in V < String] < Hash[Integer, V]
class Foo < Hash

Other directives:


  # @rbs (name:String, addresses: Array[String]) -> void
  def initialize(name:, addresses:)

This is currently available as

#:: (name: String, addresses: Array[String]) -> void
def initialize(name:, addresses:)
ParadoxV5 commented 5 months ago

Perhaps the idea is to ease migration from projects already using yard, but IMO this only makes it more complex, both to use and maintain, and the problem of yard has already been solved (sord can already translate yard to rbs).

Consider IDE integration

I don’t recall any explicit documentation, but steep watch will run a steep check everytime your file changes. Except this means that this is Steep’s workaround to the bad-for-local-CI problem. Similarly, steep langserver is a fully-fledged LSP server for IDE integration.

monkeypatch bundler

  1. We plan to merge the syntax to rbs-gem

https://github.com/soutaro/rbs-inline/issues/8#issuecomment-2095084594

ParadoxV5 commented 5 months ago

👍 abbreviations

I like your intention, but it may unfortunately be impractical, requiring to watch for yet another syntax (after # @rbs …, #:: … and (interestingly) #[…]).

Note that the current design uses #:: for many other places:

#:: () -> String
def to_s

def ==(other) #:: bool

attr_reader :name #:: String

Foo = [1,2,3].sample #:: Integer

Though, this repeatedly-reüsed #:: could use more consistent meanings/semantics. Is it the return type of calling this construct (String), or the type of this construct itself (() -> String)?

soutaro commented 5 months ago

Thanks you all for the feedback.

I don't think we will go only with the method type syntax.

# @rbs () -> String
def foo = ""

def foo = "" #:: -> String

The method name can be too long to embed in Ruby code. Or we already struggle with too long method types written in RBS files.

About IDE integration, the current rbs-inline gem is a prototype to test the syntax and to collect feedbacks. So, the implementation will be merged to rbs-gem and LSP on Steep or maybe Ruby LSP will provide direct support for them.

I'm good to add # @rbs [method-type] syntax. And #:: [method-type] syntax after def would make sense too.

soutaro commented 5 months ago

The full doc-style # @rbs should maintain a consistent identifier: type format, as if type checkers like Steep can repurpose the syntax for local variables. [P.S. No more the need to escape conflicting arg names.]

I'm getting convinced for this.

# @rbs super-class: Array[Integer]
# @rbs module-self: BasicObject
# @rbs module-self: _Each[String]
# @rbs type-params: [A, B < String, unchecked out C]
# @rbs yield: (Integer) -> String
# @rbs return: void

👍 for these with prefixes.

# @rbs *args: Integer      -- Instead of args: Array[Integer]
# @rbs **kwargs: String  -- instead of kwargs: Hash[Symbol, String]
# @rbs &block: (String) -> void  -- instead of block: ^(String) -> void

# @rbs *: Integer -- the unnamed version would make sense
# @rbs **: Integer
# @rbs &: () -> void

The last one is overriding, which looks wired to me.

# @rbs ...

And # @rbs skip will be left...

ParadoxV5 commented 5 months ago

About IDE integration, the current rbs-inline gem is a prototype to test the syntax and to collect feedbacks. So, the implementation will be merged to rbs-gem and LSP on Steep or maybe Ruby LSP will provide direct support for them.

Ah. You should clarify your intent on the README.md; I (and likely other early fans too) thought that this’ll be a (possibly stable) gem for a significant while (before the rest of the Ruby/RBS team approves the merger)!

# @rbs super-class: Array[Integer]
# @rbs module-self: BasicObject
# @rbs module-self: _Each[String]
# @rbs type-params: [A, B < String, unchecked out C]
# @rbs yield: (Integer) -> String

Hey look, utilization of Ruby keywords can make these more succinct (and cute, without hurting readability much)!

# @rbs super: Array[Integer]
# @rbs self: BasicObject
# @rbs self: _Each[String]
# @rbs class: [A, B < String, unchecked out C]
# @rbs yield: (Integer) -> String

The last one is overriding, which looks wired to me.

# @rbs ...

Consider, this annotation is to generate:

def <method>: ...

Although, # @rbs ... might be cryptic (and no less cryptic than the above RBS snippet). #:: ... seems fine because #:: is already cryptic.

ParadoxV5 commented 4 months ago

Ah. You should clarify your intent on the README.md; I (and likely other early fans too) thought that this’ll be a (possibly stable) gem for a significant while (before the rest of the Ruby/RBS team approves the merger)!

ParadoxV5 commented 3 months ago
  # @rbs (name:String, addresses: Array[String]) -> void
# @rbs return: void
# @rbs *args: Integer      -- Instead of args: Array[Integer]
# @rbs **kwargs: String  -- instead of kwargs: Hash[Symbol, String]
# @rbs &block: (String) -> void  -- instead of block: ^(String) -> void

# @rbs *: Integer -- the unnamed version would make sense
# @rbs **: Integer
# @rbs &: () -> void

TBD

# @rbs super: Array[Integer]
# @rbs self: _Each[String]
# @rbs class: [A, B < String, unchecked out C]
# @rbs yield: (Integer) -> String
# @rbs ...

And # @rbs skip will be left...

soutaro commented 3 months ago

I decided to go without having those syntaxes.

# @rbs super: Array[Integer]
# @rbs self: _Each[String]
# @rbs class: [A, B < String, unchecked out C]
# @rbs yield: (Integer) -> String

@rbs inherits T, @rbs module-self T, and @rbs generic T will be left.

We won't have @rbs yield: T, but we have @rbs &block: METHOD-TYPE.

Will add @rbs ... and #: ....

soutaro commented 3 months ago

Closing this issue because all of the items are implemented or rejected.