Keep a modifier's statement on its line for backtraces only · jruby/jruby@e9b5468 · GitHub
Skip to content

Commit e9b5468

Browse files
sferiksferik
committed
Keep a modifier's statement on its line for backtraces only
To report the right line for `foo "a", "x\ny" unless cond` (#9293), fixpos makes the statement of a modifier conditional a newline node. With coverage on, that made it a line event of its own, counted on a line the parser never marked: a multi-line statement before its condition, as in `foo 1,\n 2 if y`, read 0 on its first line where MRI reports nil. A node can now be a newline for backtraces only: the IR builder restores its line without a coverage event, and Coverage.line_stub skips it. In an interpolation, prism unsets the newline flag of a lone statement even when it is a conditional. The conditional's statements remain line events, and a modifier's statement, whose own event the conditional's otherwise hides, then counts: `"#{n} item#{'s' unless n == 1}"` counts its line once more, as in MRI.
1 parent 89c158f commit e9b5468

6 files changed

Lines changed: 64 additions & 9 deletions

File tree

‎core/src/main/java/org/jruby/ast/LineStubVisitor.java‎

Lines changed: 1 addition & 1 deletion

‎core/src/main/java/org/jruby/ast/NilImplicitNode.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ public boolean isNil() {
2626
public void setNewline() {
2727
}
2828

29+
@Override
30+
public void setBacktraceNewline() {
31+
}
32+
2933
@Override
3034
public void setLine(int line) {
3135
}

‎core/src/main/java/org/jruby/ast/Node.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ public abstract class Node {
6969
// so that the two values of a end up being different.
7070
protected boolean containsVariableAssignment;
7171
protected boolean newline;
72+
// newline only to keep backtraces on this node's line, which is no line event (see setBacktraceNewline)
73+
private boolean backtraceNewline;
7274

7375
public Node(int line, boolean containsAssignment) {
7476
this.line = line;
@@ -77,11 +79,31 @@ public Node(int line, boolean containsAssignment) {
7779

7880
public void setNewline() {
7981
this.newline = true;
82+
this.backtraceNewline = false;
83+
}
84+
85+
/**
86+
* Make a node that is no line event of its own a newline anyway, so that backtraces report its line, as for
87+
* the body of a modifier conditional written on lines before its condition. Coverage gives it no line event.
88+
*/
89+
public void setBacktraceNewline() {
90+
if (newline) return;
91+
92+
this.newline = true;
93+
this.backtraceNewline = true;
94+
}
95+
96+
/**
97+
* Whether this newline node is a line event (one Coverage counts), rather than a newline for backtraces only.
98+
*/
99+
public boolean isLineEvent() {
100+
return newline && !backtraceNewline;
80101
}
81102

82103
// Used by heredoc dedent processing. It gets unset so we do not liter line events because of it.
83104
public void unsetNewline() {
84105
this.newline = false;
106+
this.backtraceNewline = false;
85107
}
86108

87109
public boolean isNewline() {

‎core/src/main/java/org/jruby/ir/builder/IRBuilderAST.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,9 @@ private NotCompilableException notCompilable(String message, Node node) {
178178

179179
private Operand buildOperand(Variable result, Node node) throws NotCompilableException {
180180
if (node.isNewline()) {
181-
if (coverageMode != 0 && !(node instanceof NilImplicitNode)) {
181+
if (coverageMode != 0 && !node.isLineEvent()) {
182+
determineIfWeNeedLineNumberForCall(node.getLine(), true); // only for backtraces
183+
} else if (coverageMode != 0 && !(node instanceof NilImplicitNode)) {
182184
determineIfWeNeedCoverageLine(node.getLine(), LineEvents.firstInstruction(node));
183185
} else {
184186
determineIfWeNeedLineNumber(node.getLine(), true, node instanceof NilImplicitNode, node instanceof DefNode);

‎core/src/main/java/org/jruby/parser/RubyParserBase.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -789,17 +789,26 @@ public Node make_list(Node list, int line) {
789789

790790
/**
791791
* CRuby's nd_unset_fl_newline, for the statements of a string interpolation: a lone statement there is not a
792-
* line event of its own; the string it is part of is. Several statements each remain one, and so does a lone
793-
* conditional, as MRI counts its branches.
792+
* line event of its own; the string it is part of is. Several statements each remain one. A lone conditional
793+
* is no line event either, but the statements of its branches still are.
794794
*
795795
* CRuby's compiler finds coverable lines from the newline flag, but JRuby marks them as it parses, and
796796
* newline_node marked this statement's line already, so that is undone too.
797797
*/
798798
public void nd_unset_fl_newline(Node node) {
799-
if (node instanceof IfNode) return;
800-
801799
uncover(node);
802800
node.unsetNewline();
801+
802+
// In MRI the statement of a modifier conditional is a line event too, which the conditional's own (on the
803+
// same line, where both start) hides. With the conditional no line event, the statement's shows.
804+
if (node instanceof IfNode ifNode && ifNode.getSourceBody() != null) {
805+
for (Node body : new Node[] { ifNode.getThenBody(), ifNode.getElseBody() }) {
806+
if (body == null || body instanceof NilImplicitNode) continue;
807+
808+
body.setNewline();
809+
cover(body);
810+
}
811+
}
803812
}
804813

805814
// This is the last node made in the AST unintuitively so so post-processing can occur here.
@@ -1767,9 +1776,10 @@ public void fixpos(Node node, Node orig) {
17671776
// Detect IfNode and propagate newline to the bodies.
17681777
// This is a bit of a form-fitted fix, but the full reduce_nodes logic from CRuby
17691778
// defied an initial porting attempt. See jruby/jruby#9293.
1770-
if (node.isNewline() && node instanceof IfNode ifNode) {
1771-
if (ifNode.getThenBody() instanceof Node thenNode) thenNode.setNewline();
1772-
if (ifNode.getElseBody() instanceof Node elseNode) elseNode.setNewline();
1779+
// MRI gives the bodies no line events of their own, so they only keep backtraces on their lines.
1780+
if (node instanceof IfNode ifNode) {
1781+
if (ifNode.getThenBody() instanceof Node thenNode) thenNode.setBacktraceNewline();
1782+
if (ifNode.getElseBody() instanceof Node elseNode) elseNode.setBacktraceNewline();
17731783
}
17741784
}
17751785

‎test/jruby/test_coverage.rb‎

Lines changed: 17 additions & 0 deletions

0 commit comments

Comments
 (0)