Sorbet: newline inserted between sig and method

[bmulholland]

I'm adopting sorbet and adding method signatures. Since these are connected things, I expect them to be grouped together, as you can see in the Sorbet docs: https://sorbet.org/docs/sigs. However, when formatting with rufo, a newline is inserted:

% rufo
# Input
class MyObj
  sig {params(x: Integer).void}
  def initialize(x)
    @y = x  # y has the inferred type Integer
  end
end
# ctrl-d: Output
class MyObj
  sig { params(x: Integer).void }

  def initialize(x)
    @y = x  # y has the inferred type Integer
  end
end

[bmulholland]

Happy to attempt a PR for this if there's consensus that rufo should behave this way, and ideally with a pointer or two for someone new to the project's code.

[gingermusketeer]

@bmulholland Definitely makes sense to group them together in that case. A PR would be great :)

I would suggest to add a new test file for sorbet type definition examples and follow your nose from there.

[bmulholland]

I managed to create a test fine, but a 4000 line ruby file is a little overwhelming.

I'm guessing that I'm looking for something that inserts a newline between a ?command? and a method call? So maybe in visit_method? Or is it consume_space_after_command_name?

Do you have any tips on how to trace through the commands? Should I just drop a breakpoint at some point? If I do that, how do I know what commands I'm currently parsing?

[gingermusketeer]

@bmulholland Definitely a bit overwhelming. Be nice to break it up a bit. Did start working on refactoring it but that was also trying to implement a new approach for formatting.

I would try to condense the test to as little code as possible to start with so the AST is small as well. Then put a breakpoint in the visit method and inspect the node data being passed in. Keep an eye on @output as this is where the resulting formatted contents are held and skip over calls to the visit function until you see the newline being added. Can then drill down from there.

I had a bit of a look but did not find where the newline was coming from. Doesn't look like it is coming from visit_method which is what I would have expected.

Feel free to push up a branch with any questions and I can take a look.

[bmulholland]

Thanks for your tips, that was really helpful.

The root cause is here. (And the comment is super obvious once I found it!) That conditional returns true, which then adds a second new line after consume_end_of_line has already added one itself.

I'm trying to figure out what the proper fix for this would be. I feel like needs_two_lines? should probably return false in this scenario - what do you think? Problem is that the function would then need to be aware of the previous line. So I could an optional prev_expr argument for needs_two_lines?. Then, when needs_two_lines? is evaluating a :def, it can check the prev_expr and return false if prev_expr is a sig call.

Do you think this is a good approach?

If so, here's the prev_expr I get, at least for my basic test:

[[:method_add_block, [:method_add_arg, [:fcall, [:@ident, "sig", [1, 0]]], []], [:brace_block, nil, [[:method_add_arg, [:fcall, [:@ident, "returns", [1, 5]]], [:arg_paren, [:args_add_block, [[:var_ref, [:@const, "Integer", [1, 13]]]], false]]]]]], [:def, [:@ident, "foo", [2, 4]], [:params, nil, nil, nil, nil, nil, nil, nil], [:bodystmt, [[:@int, "1", [2, 9]]], nil, nil, nil]]]

What's a robust way to parse this? I feel like doing prev_expr[1][1][1][1] == "sig" is not going to be solid, but I don't know enough about this data structure to do any better.

[marras]

@bmulholland A quick hack for preventing extra line after sig calls is to add the following code in line #541 of formatter.rb.

NOTE: Like you said, this approach does not look robust at all; it boils down to removing the newline after the sig line while still allowing the def line to add another one ;). The upside is that you don't need to store/pass around any prev_expr, so the changes to the original code are minimal.

Additionally it would be nice to first detect if extend T::Sig was included somewhere in the source file before treating sig calls in a special manner. I'm definitely not going to make a PR out of this as this hack doesn't meet any standards but it works well enough for me, so I'll just leave it here :D

if exp[0] == :method_add_block and exp[1][0] == :method_add_arg and exp[1][1][0] = :fcall and exp[1][1][1][1] == "sig"
  @output.delete_suffix!("\n")
end

[bmulholland]

Thanks for the code @marras. I agree with you - this should probably trigger only when T::Sig is included. Sounds hard, though.

I suppose your usage is also a +1 for adding this feature into rufo :)

[dogweather]

I landed here because of this issue. It looks like Rufo can't really be used when there are Sorbet annotations. E.g., here's an example of Rufo-formatted code:

    sig { returns(Statute) }

    def s
      T.cast(__getobj__, Statute)
    end

    sig { params(params: T.untyped).returns(String) }

    def beautify_my_html(params)
      PublicLaw::Html.beautify_html(body, mark: params[:highlight], hide: params[:hide])
    end

    sig { returns(String) }

    def to_s
      short_citation + " " + s.name
    end

    sig { returns(String) }

    def short_name
      Base.shorten(name)
    end

Maybe the logic for fixing this could be simpler if there was a using_sorbet configuration option? Not a style setting per se; just a flag to be less opinionated about vertical spacing.

[FooBarWidget]

I would also like to voice my support for this issue.