cucumber/gherkin/ruby 13.0.0 Gherkin::Parser instantiates Gherkin::AstBuilder with wrong arguments

[BillyRuffian]

Summary

https://github.com/cucumber/cucumber/blob/master/gherkin/ruby/lib/gherkin/parser.rb#L59 Parser's initialize with no arguments passed creates a new AST Buider with no args.

https://github.com/cucumber/cucumber/blob/master/gherkin/ruby/lib/gherkin/ast_builder.rb#L6 AST Builder takes a mandatory argument.

Expected Behavior

Expected an initialized Parser object.

Current Behavior

ArgumentError: wrong number of arguments (given 0, expected 1)
	  ruby/2.6.0/gems/cucumber-gherkin-13.0.0/lib/gherkin/ast_builder.rb:6:in `initialize'
	  ruby/2.6.0/gems/cucumber-gherkin-13.0.0/lib/gherkin/parser.rb:59:in `new'
	  ruby/2.6.0/gems/cucumber-gherkin-13.0.0/lib/gherkin/parser.rb:59:in `initialize'

Possible Solution

AST Builder's initializer should take an option argument with a default id generator.

Steps to Reproduce (for bugs)

1. Gherkin::Parser.new

Context & Motivation

Using the parser to scan feature files for compliance. We've down-graded to Cucumber 3.2.

[aslakhellesoy]

Thanks @BillyRuffian the signatures are clearly wrong here. Would you be able to provide a full stack trace? That would help us better understand how this bug could have gone unnoticed while running Cucumber's own tests.

[aslakhellesoy]

From what I can tell, we're instantiating the parser here:

https://github.com/cucumber/cucumber/blob/dbd164eaff8edbda9330e69d75912b2901ddc31c/gherkin/ruby/lib/gherkin/stream/parser_message_stream.rb#L15

Since we're passing a correctly instantiated instance of AstBuilder here I wouldn't expect the default argument to be instantiated here:

https://github.com/cucumber/cucumber/blob/dbd164eaff8edbda9330e69d75912b2901ddc31c/gherkin/ruby/lib/gherkin/parser.rb#L59

This is why a full stack trace would be helpful. Where is the code that calls Parser.new() without an explicit ast builder argument?

[BillyRuffian]

Hi @aslakhellesoy, I'm creating a parser instance in this gem https://github.com/BillyRuffian/chutney/blob/master/lib/chutney.rb#L94 which we use to lint feature files so it's outside of running the cucumber framework per se.

[enkessler]

This looks similar to what's happening in #1063

[aslakhellesoy]

@BillyRuffian aha I see! You could try this then:

id_generator = Cucumber::Messages::IdGenerator::UUID.new
Parser.new(AstBuilder.new(id_generator))

I'm not a big fan of default arguments, so my preference would be to remove it. That would require a new major release.

# Remove the default argument
def initialize(ast_builder)

Alternatively we could keep it and do it correctly:

# Keep the default argument
def initialize(ast_builder = AstBuilder.new(Cucumber::Messages::IdGenerator::UUID.new))

Despite my dislike for default arguments I think we should keep it here. The reason being that it only requires a patch release, and therefore wouldn't require a whole new cucumber release.

[BillyRuffian]

@aslakhellesoy yep I'm with you and it'll be a quick patch on my side and the alternative's default arg is pretty vile. Nonetheless, the public API should work as advertised so I'm still lobbying for a patch :)