Skip to content

Preserve Ruby call syntax on print instead of canonicalizing it - #8510

Open
jkschneider wants to merge 1 commit into
mainfrom
ruby-preserve-call-syntax
Open

Preserve Ruby call syntax on print instead of canonicalizing it#8510
jkschneider wants to merge 1 commit into
mainfrom
ruby-preserve-call-syntax

Conversation

@jkschneider

Copy link
Copy Markdown
Member

What's changed

Three ways of writing a Ruby call parsed fine but printed back as a different, canonical spelling of the same call. Parser.requirePrintEqualsInput then rejected the file and it was dropped as a parse error. The shared root cause is the printer normalizing Ruby call syntax rather than preserving what the source actually wrote — in each case Prism reports the desugared call and the printer wrote that out.

Foo.() printed as Foo.call()

.() is sugar for .call(), and Prism names the call call either way. A new ImplicitCall marker records that the message was elided, and RubyPrinter omits it. The one nuance: if a recipe has since renamed the method, the new name does have to be written out, so the printer only elides while the name is still call.

User::ReputationToken.all.find_each { |t| User::ReputationPeriod::MarkForToken.(t) }
User::ReputationPeriod::Sweep.()

Foo::method() printed as Foo.method()

Ruby permits :: as a method-call delimiter, not only as constant scope resolution. The Colon2 marker was already parsed and already honoured for ordinary invocations — J.NewClass just never consulted it, so only the ::new form was affected.

WEBrick::Log::new(log_path)   # was: WEBrick::Log.new(log_path)

Cursor desync on eq({} => 0)

java.lang.IllegalStateException: Cursor desync in <file>: expected to be at offset 835
  (start of AssocNode) but was at 836

A brace-less hash whose first key is itself a hash also starts with {, and that brace was consumed as if it opened the outer hash. The brace only belongs to the hash when it sits ahead of the first pair.

Impact

Found empirically by building a corpus of 105 real Rails applications. The first case alone accounted for roughly 85% of all Ruby parse errors across that corpus and also affects discourse, gumroad, both Basecamp apps, cyberark/conjur, diaspora and dradis-ce. Measured with RubyCorpusTest on exercism/website:

files parsed failed
before 2826 1762 (62.3%) 1064
after 2826 2825 (100.0%) 1

The one remaining failure is scripts/setup_part_2.rb, which Prism itself rejects as a syntax error.

The three real-world files that produced these reports — exercism/website/test/test_helper.rb, alphagov/e-petitions/features/support/ssl_server.rb and alphagov/email-alert-api/spec/lib/collectors/global_prometheus_collector_spec.rb — all parse and round-trip.

Tests

MethodInvocationTest#callSugar, #callSugarWithBlock, #explicitCall, #colon2Call and HashTest#hashKey, all asserting the source round-trips unchanged. explicitCall pins the explicitly-written .call form so eliding the message can't overreach. Full :rewrite-ruby:test is green (452 tests).

Three ways of writing a call round-tripped as a different, canonical
spelling of the same call, so `requirePrintEqualsInput` rejected the file
and it was dropped as a parse error. In each case Prism reports the
desugared call and the printer wrote that back out instead of what the
source says.

- `Foo.()` is shorthand for `Foo.call()`, and Prism names the call `call`
  either way. A new `ImplicitCall` marker records that the message was
  elided, and the printer omits it (unless a recipe has since renamed the
  method, which does have to be written out).
- `WEBrick::Log::new(path)` printed as `WEBrick::Log.new(path)`. The
  `Colon2` marker was already parsed and already honoured for ordinary
  invocations; `J.NewClass` just never consulted it.
- `eq({} => 0)` failed with a cursor desync. A brace-less hash whose first
  key is itself a hash also starts with `{`, and that brace was consumed as
  if it opened the outer hash. The brace only belongs to the hash when it
  sits ahead of the first pair.

Bug 1 alone accounted for roughly 85% of Ruby parse errors over a corpus of
105 Rails applications; exercism/website goes from 1,064 failures to 1 (a
file Prism itself rejects as a syntax error).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant