Remove all remaining skips in newline_test.rb - #4223
Draft
eregon wants to merge 5 commits into
Draft
Conversation
* 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>
…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>
Member
Author
|
Actually it's possible to match lines from RubyVM exactly for all the files being tested, done with the 2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Still to do: update
java/api/src/main/java/org/ruby_lang/prism/MarkNewlinesVisitor.java.