Remove all remaining skips in newline_test.rb - #4223
Conversation
|
Actually it's possible to match lines from RubyVM exactly for all the files being tested, done with the 2 |
|
https://github.com/ruby/prism/actions/runs/34165671547/job/101876103431?pr=4223 fails, yet |
|
Claude found why it differs:
Wow, test-unit disabling |
* 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
left a comment
Earlopain
left a comment
There was a problem hiding this comment.
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>
Actually, it's |
|
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? |
|
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 |
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
Ah interesting I thought it doesn't use the So CRuby |
|
(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) |
…st.rb The parse.y compiler eliminates a method's implicit `nil` return (its AST has an empty method body), so it emits no :line event and records no line coverage for that line, even though the line is executed. Prism keeps the explicit nil node and correctly emits the event, which is the behavior we want to keep. See https://bugs.ruby-lang.org/issues/22302. To keep newline_test.rb passing on the parse.y build without regressing prism, add the method-implicit-nil lines to the expected bytecode events on parse.y only, recursing through tail-position conditional branches to find them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Related: #4217
This removes all skips, by fixing the logic for marking newline and generalizing the only 2 cases where it differs from RubyVM.
Regarding ruby/ruby@f4813a3 it seems the skip was simply not needed, see the first commit, which I verified passes on Ruby master and 4.0.6.
java/api/src/main/java/org/ruby_lang/prism/MarkNewlinesVisitor.java.