Add return types for callbacks in Ripper::SexpBuilder

[apiology]

No description provided.

[apiology]

I wasn't quite sure how to test callbacks like these with your tooling - open to ideas!

[apiology]

These two patterns of callbacks are defined here:

https://github.com/ruby/ruby/blob/master/ext/ripper/lib/ripper/sexp.rb#L116-L131

[apiology]

This change is limited as I'm not terribly familiar with Ripper. These changes will help https://github.com/castwide/solargraph pass its own typechecking; here's the use it makes of Ripper: https://github.com/castwide/solargraph/blob/41b892397be99fd6b445d1e308a3f512c5ec32e9/lib/solargraph/parser/comment_ripper.rb#L4

cc: @aamine in case you have thoughts

[apiology]

Should I push up additional fixes to RBS types?

[ksss]

@apiology
Thank you for requesting.
There are many changes and we are finding it difficult to confirm each one.
Can you provide a test code?

[apiology]

I'm sorry - I don't know how to test callbacks like these with your tooling. Do you have any ideas? A small example and I can fill in the rest.

[apiology]

If it helps, I am successfully using my fork of this repo to typecheck this file: https://github.com/castwide/solargraph/blob/0e82bbdcc9da7343dd028373797f2fe0373dbf6a/lib/solargraph/parser/comment_ripper.rb

[ksss]

Since the return values of all callback methods are never used, untyped or void seems appropriate.
Callback methods are meant to be overridden in subclasses, so perhaps they shouldn't be defined at all.

I wrote the original signatures myself, but I honestly have no memory of it...

[apiology]

Since the return values of all callback methods are never used, untyped or void seems appropriate.
Callback methods are meant to be overridden in subclasses, so perhaps they shouldn't be defined at all.

The return values actually are used - they determine what is returned by the overall parse() method - see https://github.com/ruby/ruby/blob/a4a60195502add094fb52a587411bbd0c19facce/ext/ripper/lib/ripper/filter.rb#L59

As you note, Ripper::SexpBuilder is used by subclassing. The return types indicate to the developer what they must return from the methods they write as well as what information they can gain from the superclass method's return value.

Here is the code I would like to be well-typed: https://github.com/castwide/solargraph/blob/0e82bbdcc9da7343dd028373797f2fe0373dbf6a/lib/solargraph/parser/comment_ripper.rb - and here is how the resulting code is used: https://github.com/castwide/solargraph/blob/767a5fceb74935d03f6757711bdf610df3e6fd7a/lib/solargraph/parser/parser_gem/class_methods.rb#L17

I wrote the original signatures myself, but I honestly have no memory of it...

I am very familiar with that feeling 😁

[ksss]

Ripper::Filter

After looking into it, it seems that for Ripper::Filter, it would be appropriate to restrict the on_* methods to return the same type as the second data argument.

# ripper.rbs
class Ripper
  class Filter[T = untyped]
    def on_kw: (untyped tok, T data) -> T
  end
end

class Foo < Ripper::Filter[Array[String]]
end

# foo.rb
class Foo < Ripper::Filter
  def on_kw(tok, data)
    # `data` is `T(Array[String])`
    data << "aaa"
    nil # typecheck should fail because `on_kw` return type is `T(Array[String])`
  end
end

Ripper::SexpBuilder and Ripper::SexpBuilderPP

As for Ripper::SexpBuilder, Even though the return value is not actually used, adding type constraints to the overridden method poses a risk of compromising practicality.
In the first place, Ripper::SexpBuilder is an undocumented internal class, so I think most of its methods shouldn’t be explicitly defined at all.