Fix SyntaxError when instrumenting hash with shorthand

[kxmbrian]

Why

We get a SyntaxError when instrumenting hashes with Ruby 3.1 hash shorthand syntax in controllers.

Fixes #484
Fixes #445

Fix

Add a on_hash method that does the instrumentation correctly.

@jrothrock - would you be the right person to ask for a review? Do let me know if there's a cleaner way to do this.

Testing

I've added test cases.

@natematykiewicz - if you would like to, you may test this with your app.

[jrothrock]

Thanks for the PR and the included test cases!

I stepped through this with a locally reproduced issue of this, and it fixed it and looks good!

It does look like this requires parser version 3.0.3.0 and above, maybe we need to mention this in the docs, but with the general parser warning logs related to the Ruby version maybe we're good?

[natematykiewicz]

Perhaps you want to set a minimum required parser version? Rubocop recently had to do that:
rubocop/rubocop@55e9e70

[jrothrock]

I'm not sure this is something we can do right now unfortunately.

We still support Ruby versions 2.1 - 2.7, even though they have been EOL, and parser may not maintain backward compatibility for older Ruby versions and throws a warning about version compliance

[kxmbrian]

Since:

Parser versions are structured as x.y.z.t, where x.y.z indicates the most recent supported Ruby release (support for every Ruby release that is chronologically earlier is implied), and t is a monotonically increasing number.

Would it be reasonable to do this?

s.add_runtime_dependency "parser", "~> #{RUBY_VERSION}.0"

In general, I'd think it's an anti-pattern to depend on RUBY_VERSION in the gemspec, but it could be acceptable in this case since one would expect an APM library that instruments Ruby to be tailored to the version of Ruby that it's instrumenting, i.e. a strong coupling to RUBY_VERSION is already expected. Any thoughts?

[kxmbrian]

Pushed a commit with this instead because not every patch-level Ruby version has a corresponding parser gem version (see available versions):

s.add_runtime_dependency "parser", Gem::Version.new(RUBY_VERSION).approximate_recommendation

Looks better without any string manipulation and the tests pass. Here's an example of the output string:

> Gem::Version.new(RUBY_VERSION).approximate_recommendation
"~> 3.3"

[natematykiewicz]

I'll bet this explains why I've had auto instrumentation not work at times and then later magically work -- I had upgraded Ruby without upgrading Parser, resulting in Parser being incompatible with my Ruby version. Eventually I update some gem that relies on Parser (Scout, Rubocop) and get a version of Parser that actually supports my Ruby version and auto instrumentation starts working again.

[natematykiewicz]

Per https://bundler.io/guides/gemfile.html

Most of the version specifiers, like >= 1.0, are self-explanatory. The specifier ~> has a special meaning, best shown by example. ~> 2.0.3 is identical to >= 2.0.3 and < 2.1. ~> 2.1 is identical to >= 2.1 and < 3.0. ~> 2.2.beta will match prerelease versions like 2.2.beta.12. ~> 0 is identical to >= 0.0 and < 1.0.

Suggested change

	s.add_runtime_dependency "parser", Gem::Version.new(RUBY_VERSION).approximate_recommendation
	s.add_runtime_dependency "parser", "#{Gem::Version.new(RUBY_VERSION).approximate_recommendation}.0"

Since Parser has a version that corresponds to every minor version, we want ~> 3.2.0 which means >= 3.2.0 and < 3.3. Right now you've got ~> 3.2 which means >= 3.2 and < 4.0. But I don't think we want to use Parser 3.3 on Ruby 3.2.

[kxmbrian]

Good catch, thanks!

[kxmbrian]

Hmm getting these errors for the tests

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.3-compliant syntax, but you are running 2.3.8.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
rake aborted!
LoadError: Parser::TreeRewriter was not defined

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.10.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
rake aborted!
LoadError: Parser::TreeRewriter was not defined

The last Parser 2.3 is Parser 2.3.3.1. Looks like we're expected to use Parser 2.4 for Ruby 2.3.8.

But I don't think we want to use Parser 3.3 on Ruby 3.2.

Agree with you that this is not ideal, but it's probably safe enough. The main thing is to get folks to upgrade the parser whenever they upgrade their Ruby version.

Gonna revert for now.

[jrothrock]

Thanks for the continued effort on this guys. We appreciate it!

I think this is good.

This also may solve some issues related to:
#445

But still allows people below Ruby 2.5 to get to a parser version above 2.5 which has the TreeRewriter. Again also not ideal, but probably safe