Add underlining to safe-init stack traces #14683

Xavientois · 2022-03-13T23:00:57Z

This underlines the source positions in the safe-init stack traces similarly to how the MessageRendering will underline its stack positions.

Before, a safe-init error might look like this:

[error] -- Error: /******/dotty/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala:1032:22
[error] 1032 |      "inlineVars" -> inlineVars
[error]      |                      ^^^^^^^^^^
[error]      |Cannot prove that the value is fully initialized. Only initialized values may be used as arguments
[error]      |
[error]      |The unsafe promotion may cause the following problem:
[error]      |Calling the external method method apply may cause initialization errors. Calling trace:
[error]      | -> "inlineVars" -> inlineVars	[ PatternMatcher.scala:1032 ]
[error]      |  -> Inliner(plan)	[ PatternMatcher.scala:700 ]
[error]      |   -> case plan: TestPlan => apply(plan)	[ PatternMatcher.scala:492 ]
[error]      |    -> plan.scrutinee = apply(plan.scrutinee)	[ PatternMatcher.scala:472 ]
[error]      |     -> def apply(tree: Tree): Tree = treeMap.transform(tree)	[ PatternMatcher.scala:470 ]
[error]      |      -> if (toDrop(sym)) transform(initializer(sym))	[ PatternMatcher.scala:675 ]

Now, the same error looks like this:

[error] -- Error: /******/dotty/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala:1032:22
[error] 1032 |      "inlineVars" -> inlineVars
[error]      |                      ^^^^^^^^^^
[error]      |Cannot prove that the value is fully initialized. Only initialized values may be used as arguments
[error]      |
[error]      |The unsafe promotion may cause the following problem:
[error]      |Calling the external method method apply may cause initialization errors. Calling trace:
[error]      |-> "inlineVars" -> inlineVars	[ PatternMatcher.scala:1032 ]
[error]      |                   ^^^^^^^^^^
[error]      |-> Inliner(plan)	[ PatternMatcher.scala:700 ]
[error]      |   ^^^^^^^^^^^^^
[error]      |-> case plan: TestPlan => apply(plan)	[ PatternMatcher.scala:492 ]
[error]      |                          ^^^^^^^^^^^
[error]      |-> plan.scrutinee = apply(plan.scrutinee)	[ PatternMatcher.scala:472 ]
[error]      |                    ^^^^^^^^^^^^^^^^^^^^^
[error]      |-> def apply(tree: Tree): Tree = treeMap.transform(tree)	[ PatternMatcher.scala:470 ]
[error]      |                                 ^^^^^^^^^^^^^^^^^^^^^^^
[error]      |-> if (toDrop(sym)) transform(initializer(sym))	[ PatternMatcher.scala:675 ]

This should improve the readability of the safe-init error messages.

Review by @liufengyun

olhotak · 2022-03-14T10:00:59Z

There are some failures on the community build.

I like this and could merge after those are fixed, but I wonder if we should discuss a couple things.

First, does indenting each line a bit more than the previous line still make sense?

Second, can we add some text to each line indicating the relationship of each line to the next (i.e. method call, field read, inlined method, etc.)? That would probably be a separate PR though.

@liufengyun What do you think?

compiler/src/dotty/tools/dotc/transform/init/Errors.scala

Xavientois · 2022-03-14T12:18:48Z

First, does indenting each line a bit more than the previous line still make sense?

I still like the indenting, but it does appear less necessary for legibility given the increased spacing after this change. I don't think it hurts to keep it, though. If you think it makes more sense to remove it, I can do that in this PR or it could happen in a separate PR after the fact.

Second, can we add some text to each line indicating the relationship of each line to the next (i.e. method call, field read, inlined method, etc.)? That would probably be a separate PR though.

I am planning to try implementing this in a separate PR.

smarter · 2022-03-14T12:34:44Z

similarly to how the MessageRendering will underline its stack positions.

Could MessageRendering be re-used instead? It supports displaying a stack of lines since #14002 /cc @nicolasstucki

Xavientois · 2022-03-14T12:40:33Z

similarly to how the MessageRendering will underline its stack positions.

Could MessageRendering be re-used instead? It supports displaying a stack of lines since #14002 /cc @nicolasstucki

I considered implementing it that way, but due to the indentation and trimming of the source lines in the trace, all but one of the lines of the positionMarker function were different.

If we wanted to re-use MessageRendering to completely handle this case, it would require a more involved re-work of the safe-init error message code and the MessageRendering code. The implementation from MessageRendering appears to specifically handle the case of inlined function calls, so it would need to be modified to handle the special case of the safe-init trace. We would also need to modify the implementation of Diagnostic to be able to store a stack of positions rather than just a single SourcePosition. I would be open to trying to implement this, but it would be beyond the scope of this PR.

liufengyun

LGTM, thank you @Xavientois 🎉

First, does indenting each line a bit more than the previous line still make sense?

I'm open to remove it if it is not useful.

Xavientois · 2022-03-18T19:03:08Z

compiler/src/dotty/tools/dotc/util/SourceFile.scala

@@ -211,8 +211,8 @@ class SourceFile(val file: AbstractFile, computeContent: => Array[Char]) extends
  def startColumnPadding(offset: Int): String = {
    var idx = startOfLine(offset)
    val pad = new StringBuilder
-    while (idx != offset) {
-      pad.append(if (idx < length && content()(idx) == '\t') '\t' else ' ')
+    while (idx != offset && idx < length && idx < content().length) {


This handles the case when content().length != length

@nicolasstucki Could you have a look at this change?

I don't see how content().length != length is possible. calculateLineIndicesFromContents() has:

val cs = content() ... buf += cs.length

and length calls lineIndicesCache.last.

@Xavientois do you have a concrete test case for which content().length != length?

It seems content().length != length happens when length is computed from Tasty using setLineIndicesFromLineSizes instead of using calculateLineIndicesFromContents.

olhotak · 2022-03-30T14:37:09Z

@Xavientois : After rebasing the PR on main and git submodule update, I was able to reproduce the exception locally:
[error] java.lang.ArrayIndexOutOfBoundsException: Index 3375 out of bounds for length 0

@liufengyun

This underlines the source positions in the safe-init stack traces similarly to how the MessageRendering will underline its stack positions. Before, a safe-init error might look like this: ``` [error] -- Error: /******/dotty/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala:1032:22 [error] 1032 | "inlineVars" -> inlineVars [error] | ^^^^^^^^^^ [error] |Cannot prove that the value is fully initialized. Only initialized values may be used as arguments [error] | [error] |The unsafe promotion may cause the following problem: [error] |Calling the external method method apply may cause initialization errors. Calling trace: [error] | -> "inlineVars" -> inlineVars [ PatternMatcher.scala:1032 ] [error] | -> Inliner(plan) [ PatternMatcher.scala:700 ] [error] | -> case plan: TestPlan => apply(plan) [ PatternMatcher.scala:492 ] [error] | -> plan.scrutinee = apply(plan.scrutinee) [ PatternMatcher.scala:472 ] [error] | -> def apply(tree: Tree): Tree = treeMap.transform(tree) [ PatternMatcher.scala:470 ] [error] | -> if (toDrop(sym)) transform(initializer(sym)) [ PatternMatcher.scala:675 ] ``` Now, the same error looks like this: ``` [error] -- Error: /******/dotty/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala:1032:22 [error] 1032 | "inlineVars" -> inlineVars [error] | ^^^^^^^^^^ [error] |Cannot prove that the value is fully initialized. Only initialized values may be used as arguments [error] | [error] |The unsafe promotion may cause the following problem: [error] |Calling the external method method apply may cause initialization errors. Calling trace: [error] |-> "inlineVars" -> inlineVars [ PatternMatcher.scala:1032 ] [error] | ^^^^^^^^^^ [error] |-> Inliner(plan) [ PatternMatcher.scala:700 ] [error] | ^^^^^^^^^^^^^ [error] |-> case plan: TestPlan => apply(plan) [ PatternMatcher.scala:492 ] [error] | ^^^^^^^^^^^ [error] |-> plan.scrutinee = apply(plan.scrutinee) [ PatternMatcher.scala:472 ] [error] | ^^^^^^^^^^^^^^^^^^^^^ [error] |-> def apply(tree: Tree): Tree = treeMap.transform(tree) [ PatternMatcher.scala:470 ] [error] | ^^^^^^^^^^^^^^^^^^^^^^^ [error] |-> if (toDrop(sym)) transform(initializer(sym)) [ PatternMatcher.scala:675 ] ``` This should improve the readability of the safe-init error messages. Review by @liufengyun

olhotak

LGTM

The community build tests now pass with the fix to startColumnPadding following the same idiom as in column.

Xavientois force-pushed the underline-safe-init-trace branch 2 times, most recently from 84f8fc3 to 620d3f7 Compare March 14, 2022 00:57

prolativ approved these changes Mar 14, 2022

View reviewed changes

prolativ self-requested a review March 14, 2022 09:58

prolativ previously requested changes Mar 14, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/transform/init/Errors.scala Outdated Show resolved Hide resolved

Xavientois force-pushed the underline-safe-init-trace branch from 620d3f7 to 1199e30 Compare March 14, 2022 12:29

Xavientois force-pushed the underline-safe-init-trace branch 2 times, most recently from 356a34d to 0cfbd11 Compare March 14, 2022 14:58

liufengyun approved these changes Mar 14, 2022

View reviewed changes

Xavientois force-pushed the underline-safe-init-trace branch 4 times, most recently from c74bd98 to 08bcec6 Compare March 18, 2022 17:52

Xavientois commented Mar 18, 2022

View reviewed changes

Xavientois requested a review from prolativ March 19, 2022 14:33

Xavientois force-pushed the underline-safe-init-trace branch from 08bcec6 to 286b518 Compare March 27, 2022 15:07

Xavientois mentioned this pull request Mar 27, 2022

IndexOutOfBounds in SourceFile.scala contents() #14795

Closed

Xavientois force-pushed the underline-safe-init-trace branch from 286b518 to 4c75c8d Compare April 4, 2022 18:46

Xavientois requested a review from olhotak April 4, 2022 21:03

olhotak approved these changes Apr 5, 2022

View reviewed changes

olhotak merged commit d8e50f2 into scala:main Apr 5, 2022

Xavientois deleted the underline-safe-init-trace branch April 5, 2022 14:48

Kordyjan added this to the 3.1.3 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add underlining to safe-init stack traces #14683

Add underlining to safe-init stack traces #14683

Xavientois commented Mar 13, 2022 •

edited

Loading

olhotak commented Mar 14, 2022

Xavientois commented Mar 14, 2022

smarter commented Mar 14, 2022

Xavientois commented Mar 14, 2022 •

edited

Loading

liufengyun left a comment

Xavientois Mar 18, 2022

liufengyun Mar 23, 2022

olhotak Mar 23, 2022

olhotak Mar 23, 2022

olhotak commented Mar 30, 2022

olhotak left a comment

Add underlining to safe-init stack traces #14683

Add underlining to safe-init stack traces #14683

Conversation

Xavientois commented Mar 13, 2022 • edited Loading

olhotak commented Mar 14, 2022

Xavientois commented Mar 14, 2022

smarter commented Mar 14, 2022

Xavientois commented Mar 14, 2022 • edited Loading

liufengyun left a comment

Choose a reason for hiding this comment

Xavientois Mar 18, 2022

Choose a reason for hiding this comment

liufengyun Mar 23, 2022

Choose a reason for hiding this comment

olhotak Mar 23, 2022

Choose a reason for hiding this comment

olhotak Mar 23, 2022

Choose a reason for hiding this comment

olhotak commented Mar 30, 2022

olhotak left a comment

Choose a reason for hiding this comment

Xavientois commented Mar 13, 2022 •

edited

Loading

Xavientois commented Mar 14, 2022 •

edited

Loading