Remove all remaining skips in newline_test.rb by eregon · Pull Request #4223 · ruby/prism

eregon · 2026-09-07T21:50:52Z

No skips left in newline_test.rb
No workarounds left in newline_test.rb, the logic to mark newlines matches exactly the same lines as CRuby 3.4+ (so 3.4, 4.0, master prism, master parse.y) for the files included in newline_test.rb
The corresponding Java logic is updated too
The corresponding Java logic is now tested to mark exactly the same lines

eregon · 2026-09-07T22:10:37Z

Actually it's possible to match lines from RubyVM exactly for all the files being tested, done with the 2 Match RubyVM newlines commits.
I'm not entirely sure it's a good idea, still making my mind about it and welcoming other opinions on it.

eregon · 2026-09-08T21:08:01Z

https://github.com/ruby/prism/actions/runs/34165671547/job/101876103431?pr=4223 fails, yet bundle exec ruby -Ilib test/prism/newline_test.rb passes locally on ruby-dev, very weird.
Also the failure is because line 95 of ruby/parameters_signature_test.rb is expected twice, but this makes no sense, line 95 of that file is: object = Object.new.
https://github.com/ruby/prism/actions/runs/34165671547/job/101876103431?pr=4223#step:8:869

eregon · 2026-09-08T21:19:04Z

Claude found why it differs:

require "test/unit" (the test-unit gem, 3.7.8) globally sets RubyVM::InstructionSequence.compile_option specialized_instruction: false. opt_new is a specialized instruction, so with it disabled the opt_new codepath is never generated, the duplicate :line trace point vanishes, and the test passes.

Wow, test-unit disabling specialized_instructions sounds pretty bad.

eregon · 2026-09-08T21:23:39Z

-> https://bugs.ruby-lang.org/issues/22299

* Older versions have known bugs in this area, like not emitting a :line event for `nil`, and there is no value to replicate them.

Fix two mismatches between prism's newline flags and RubyVM's line events in the Newlines visitor: * def, class, module, and singleton class nodes compile to their own ISeqs with independent line-event tracking, so reset the line table for them like blocks and lambdas already do. This matches one-line definitions like `def foo; bar; end`, where the bytecode emits two line events on the same line. The body of an endless method definition never emits newline events, so in that case mark every line as already seen instead. * Statements inside string interpolation do not emit line events, so mark every line as already seen while visiting embedded statements. Nested scopes (blocks, lambdas, defs, etc.) still reset the lines and emit events again. The remaining divergences are bytecode artifacts: for statements like `foo = [` or `foo =` where the value continues on the following lines, the line event is emitted on the line of the first sub-expression of the value instead of on the first line of the statement. Replace the two ad-hoc compensations in the test with a single count-based rule that moves or drops the newline flag accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Lines like `while (foo = bar)` result in two line events in the bytecode: parentheses make the inner expression a statement with its own line event, and the predicate of a while or until loop is compiled at the end of the loop, after the body, so that event is emitted again in addition to the one for the loop statement itself. This also mirrors runtime behavior, since the predicate line fires on each iteration. Match this in the Newlines visitor by marking the loop node itself when a prefix loop has a parenthesized predicate, and by visiting the predicate with a fresh set of lines so that its statements can mark lines that were already seen. This removes the corresponding compensation in newline_test.rb. The remaining compensation for assignments whose value continues on the following lines is kept: the line event is emitted on the line of the statement's first compiled instruction, which depends on constant folding (for example, an array of static literals compiles to a single instruction on the line of the literal, and string literals are only static under `# frozen_string_literal: true`). That is a property of the compiler rather than of the AST, so it does not belong in the newline flags. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Earlopain

Overall I'm ok with this change, nice cleanup to the skips, but I'm thinking this might overfit for the few special cases the test code actually contains. Would be fine though in my eyes, we can see how it actually goes.

…test.rb The line event for a statement is emitted where its first instruction is compiled, so make nodes whose first instruction comes from a sub-expression delegate their newline flag to that sub-expression: assignments to their value, calls to their receiver, and array, hash, and interpolated string literals to their first element. Static literals are the exception: they are compiled to a single instruction on the first line of the literal, so they do not delegate. The static literal flag captures the folding boundary exactly, including that string literals are only static under `# frozen_string_literal: true`, both in arrays and in the parts of heredocs. With this, prism's newline flags match RubyVM's line events exactly on every file in the test suite and newline_test.rb needs no compensation logic at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Earlopain · 2026-09-09T11:45:53Z

Wow, test-unit disabling specialized_instructions sounds pretty bad.

Actually, it's power_assert (which test-unit unconditonally requires)

eregon · 2026-09-09T14:11:01Z

Right, maybe we should switch to https://github.com/ruby/test-unit-ruby-core in Prism so it's the same test harness used in CRuby CI and Prism CI?

Earlopain · 2026-09-09T16:01:58Z

I'm not so familiar with it, but my understanding is that it just adds some extra assertions. I tested it and in the end power_assert still changes compile options anyways.

Originally this was done in ruby@c23908f But it is no longer needed because ruby now emits these events regardless. Reported in https://bugs.ruby-lang.org/issues/14870 and fixed with ruby/ruby@48c8df9 In ruby/prism#4223 we ran into this because it caused a difference in tracepoint line events

eregon · 2026-09-09T20:46:16Z

I'm not so familiar with it, but my understanding is that it just adds some extra assertions. I tested it and in the end power_assert still changes compile options anyways.

Ah interesting I thought it doesn't use the test-unit gem (but some kind of shim using minitest under the good), that probably changed at some point.

So CRuby test-all seems to use yet another test harness then (which doesn't change compile options).

eregon · 2026-09-09T21:02:01Z

I'm looking at the last failure, it's a divergence between the parse.y and prism compilers since 3.4, in 3.3 they agreed.
=> It's https://bugs.ruby-lang.org/issues/22302

…t.rb" Mirror the newlines.rb changes of that commit in MarkNewlinesVisitor: def, class, module, and singleton class nodes compile to their own ISeqs with independent line-event tracking, so reset the marked lines for them like blocks and lambdas already do. The body of an endless method definition and statements inside string interpolation never emit newline events, so mark every line as already seen while visiting them. Since location fields are not available in the Java nodes, an endless method definition is detected as a statements body which ends with the def node itself, which was verified to be equivalent to checking the location of the `=` operator on every def node in this repository. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…hile/until predicates" Mirror the newlines.rb changes of that commit in MarkNewlinesVisitor: the predicate of a while/until loop is compiled at the end of the loop, after the body, so any statements it contains (from parentheses) emit their line events again even if the lines were already seen, in addition to the line event of the loop itself. Since the newline_flag! overrides in newlines.rb are no longer a mirror of the generated setNewLineFlag() methods, introduce a setNewLineFlag() helper in the visitor for the extra logic. There is no keyword location in the Java nodes, but a prefix while/until loop can be detected as one which is not a begin modifier loop and does not start with its statements, which was verified to be equivalent to checking the keyword location on every while/until node in this repository. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ll compensations in newline_test.rb" Mirror the newlines.rb changes of that commit in MarkNewlinesVisitor: the line event for a statement is emitted where its first instruction is compiled, so nodes whose first instruction comes from a sub-expression delegate their newline flag to that sub-expression: assignments to their value, calls to their receiver, and array, hash, and interpolated string literals to their first element. Static literals are the exception: they are compiled to a single instruction on the first line of the literal, so they do not delegate. The delegating newline_flag! overrides in newlines.rb dispatch to each other, so the corresponding cases in the setNewLineFlag() helper of the visitor recurse through the helper, including for the predicate of if/unless/while/until and the expression of a rescue modifier. PM_NODE_FLAG_STATIC_LITERAL is serialized together with the node-specific flags, so it is read from the flags for nodes which have them, and computed structurally for hash literals (whose nodes have no flags), following the same rules as the parser: an assoc node is a static literal if neither its key nor its value is an array, hash, or range and both are static literals themselves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…:java_loader Add a check to the test:java_loader task (run by the build-java job in CI) that MarkNewlinesVisitor.java marks exactly the same lines as Prism::ParseResult#mark_newlines! (newlines.rb), for the newline_test.rb corpus and every fixture which parses successfully, so that both cannot get out of sync. The expected lines are generated by a new test:java_loader:newline_fixtures task, which runs in a subprocess with the default (full) serialization mode since newlines.rb needs location fields, before the existing clobber and recompile with PRISM_SERIALIZE_ONLY_SEMANTICS_FIELDS=1. The Java side runs in the existing task through Loader.load(), which runs MarkNewlinesVisitor, and the marked lines are collected from the loaded Java nodes directly in JRuby. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

eregon · 2026-09-11T20:19:29Z

This is now ready for review, summary (I also updated the PR description to the same):

No skips left in newline_test.rb
No workarounds left in newline_test.rb, the logic to mark newlines matches exactly the same lines as CRuby 3.4+ (so 3.4, 4.0, master prism, master parse.y) for the files included in newline_test.rb
The corresponding Java logic is updated too
The corresponding Java logic is now tested to mark exactly the same lines

eregon · 2026-09-11T20:24:56Z

For the 2 changes to logic to match CRuby:

Match RubyVM newlines for parenthesized while/until predicates 2 :line events for loops is a little bit weird, but somewhat consistent with things like methods/blocks creating a new "scope" and having a new set of lines to mark. It's also a fairly simple change.
Match RubyVM newlines exactly, removing all compensations in newline_test.rb marking the value instead of the assignment makes sense for a bytecode interpreter, and is easy, we already did some of that. The part about static literals is more subtle but also makes sense for bytecode interpreters, as the elements will be executed first before building the array (unless it's a static literal and then a single :line event where the array starts).

So while I think it's debatable whether that's the best location for :line events, matching CRuby is best for compatibility and it turns out to not be too hard either.

eregon force-pushed the newline-test branch from 95a91c3 to fed449b Compare September 7, 2026 22:09

This was referenced Sep 8, 2026

compile.c: statically resolve swap instructions when possible ruby/ruby#18538

Merged

Remove NewlineTest? #4217

Open

eregon and others added 4 commits September 9, 2026 13:33

Run newline_test.rb only on Ruby 3.4+

bac12e8

* Older versions have known bugs in this area, like not emitting a :line event for `nil`, and there is no value to replicate them.

Remove unnecessary skips in newline_test.rb

0c34d80

eregon force-pushed the newline-test branch from d9fb2b8 to 05b785a Compare September 9, 2026 11:33

Earlopain reviewed Sep 9, 2026

View reviewed changes

Comment thread test/prism/newline_test.rb Outdated

Comment thread lib/prism/parse_result/newlines.rb Outdated

eregon force-pushed the newline-test branch from 05b785a to a7dab1e Compare September 9, 2026 11:38

Add a note about what a skip means in newline_test.rb

9bf6cbc

eregon force-pushed the newline-test branch from a7dab1e to 9bf6cbc Compare September 9, 2026 14:08

Earlopain mentioned this pull request Sep 9, 2026

Don't disable specialized instructions ruby/power_assert#70

Open

eregon force-pushed the newline-test branch from 34b2ada to 9bf6cbc Compare September 10, 2026 21:45

eregon and others added 4 commits September 11, 2026 21:35

eregon force-pushed the newline-test branch from afc1f7e to 2299cc9 Compare September 11, 2026 20:17

eregon marked this pull request as ready for review September 11, 2026 20:17

eregon requested review from Earlopain and kddnewton September 11, 2026 20:25

Navigation Menu

Uh oh!

Remove all remaining skips in newline_test.rb - #4223

Remove all remaining skips in newline_test.rb#4223
eregon wants to merge 10 commits into
ruby:mainfrom
eregon:newline-test

eregon commented Sep 7, 2026 •

edited

Loading

Uh oh!

eregon commented Sep 7, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

Earlopain left a comment

Uh oh!

Uh oh!

Uh oh!

Earlopain commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026

Uh oh!

Earlopain commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026 •

edited

Loading

Uh oh!

eregon commented Sep 11, 2026 •

edited

Loading

Uh oh!

eregon commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

eregon commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented Sep 7, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

eregon commented Sep 8, 2026

Uh oh!

Earlopain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Earlopain commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026

Uh oh!

Earlopain commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026

Uh oh!

eregon commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eregon commented Sep 7, 2026 •

edited

Loading

eregon commented Sep 9, 2026 •

edited

Loading

eregon commented Sep 11, 2026 •

edited

Loading