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

Sord generates an invalid rbi file for haml #75

Open
connorshea opened this issue Jul 8, 2019 · 7 comments
Open

Sord generates an invalid rbi file for haml #75

connorshea opened this issue Jul 8, 2019 · 7 comments
Labels
bug Something isn't working external The issue is caused by a problem in a different project

Comments

@connorshea
Copy link
Contributor

Describe the bug
haml.rbi has an invalid method in it for some reason:

sig { params(index: T.untyped).returns(T.untyped) }
def rstrip_buffer!(index = -1)); end

To Reproduce
Run sord on haml and then run srb tc on the resulting rbi.

Expected behavior
It'd output a valid rbi file.

Actual behavior
It outputs this method with an extra closing parenthesis:

sig { params(index: T.untyped).returns(T.untyped) }
def rstrip_buffer!(index = -1)); end

Additional information
The problem isn't with Haml's method, as far as I can tell it's valid:

# Get rid of and whitespace at the end of the buffer
# or the merged text
def rstrip_buffer!(index = -1)
  last = @to_merge[index]
  if last.nil?
    push_silent("_hamlout.rstrip!", false)
    return
  end

  case last.first
  when :text
    last[1] = last[1].rstrip
    if last[1].empty?
      @to_merge.slice! index
      rstrip_buffer! index
    end
  when :script
    last[1].gsub!(/\(haml_temp, (.*?)\);$/, '(haml_temp.rstrip, \1);')
    rstrip_buffer! index - 1
  else
    raise SyntaxError.new("[HAML BUG] Undefined entry in Haml::Compiler@to_merge.")
  end
end

https://github.com/haml/haml/blob/a054e2ad7964f894fce032737f9a4f0e146700a5/lib/haml/compiler.rb#L308

@connorshea connorshea added the bug Something isn't working label Jul 8, 2019
@connorshea
Copy link
Contributor Author

connorshea commented Jul 8, 2019

Here's a spec:

it 'generates valid rbi' do
  YARD.parse_string(<<-RUBY)
    module A
      def x(a = -1)
        # code
      end
    end
  RUBY

  expect(subject.generate.strip).to eq fix_heredoc(<<-RUBY)
    # typed: strong
    module A
      # sord omit - no YARD type given for "a", using T.untyped
      # sord omit - no YARD return type given, using T.untyped
      sig { params(a: T.untyped).returns(T.untyped) }
      def x(a = -1); end
    end
  RUBY
end

It seems like it doesn't like negative integers?

@connorshea
Copy link
Contributor Author

Actually, it seems like this might be a YARD bug!

If you run that test and add puts "name: #{name}, default: #{default}" to the meth.parameters loop on line 138 of rbi_generator.rb, the output includes this:

name: a, default: -1)

@AaronC81
Copy link
Owner

AaronC81 commented Jul 8, 2019

Yep, that's a YARD bug:

image

Good find!

@AaronC81 AaronC81 added the external The issue is caused by a problem in a different project label Jul 8, 2019
@connorshea
Copy link
Contributor Author

Looks like we were bitten by lsegal/yard#894

@connorshea
Copy link
Contributor Author

I just got this same error again with Faker, this time it creates an extra comma as well (I guess it just duplicates the character that follows it?).

Input:

module Faker
  class Number < Base
    class << self
      def negative(legacy_from = NOT_GIVEN, legacy_to = NOT_GIVEN, from: -5000.00, to: -1.00)
        warn_for_deprecated_arguments do |keywords|
          keywords << :from if legacy_from != NOT_GIVEN
          keywords << :to if legacy_to != NOT_GIVEN
        end

        random_number = between(from: from, to: to)

        less_than_zero(random_number)
      end
    end
  end
end

Output:

sig do
  params(
    legacy_from: T.untyped,
    legacy_to: T.untyped,
    from: T.untyped,
    to: T.untyped
  ).returns(T.untyped)
end
def self.negative(legacy_from = NOT_GIVEN, legacy_to = NOT_GIVEN, from: -5000.00,, to: -1.00)); end

@connorshea
Copy link
Contributor Author

@AaronC81 I've been thinking about this since it still bothers me a bit, would it be insane to write a regex for detecting and fixing these cases?

@AaronC81
Copy link
Owner

@connorshea That sounds like a workaround worth looking into!

It'd certainly be good to have some kind of fix for this, and I guess fixing it within YARD is pretty tricky if the issue has been around since 2015.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working external The issue is caused by a problem in a different project
Projects
None yet
Development

No branches or pull requests

2 participants