Fix SourceFile.column method.
#15209
Conversation
| @@ -197,12 +194,7 @@ class SourceFile(val file: AbstractFile, computeContent: => Array[Char]) extends | |||
| /** The column corresponding to `offset`, starting at 0 */ | |||
| def column(offset: Int): Int = { | |||
| var idx = startOfLine(offset) | |||
| var col = 0 | |||
| while (idx != offset) { | |||
| col += (if (idx < content().length && content()(idx) == '\t') (tabInc - col) % tabInc else 1) | |||
There was a problem hiding this comment.
This logic also exists in Scala 2: https://github.com/scala/scala/blob/2.13.x/src/reflect/scala/reflect/internal/util/Position.scala
Is it wrong there too, or is there a bug in our implementation? Is it because of the extra parens added around tabInc - col in de1042a#diff-2314f137397e23174a2406e3e24f35f5b3d51811fb410a3272e3d42ce13624b5L121 ?
There was a problem hiding this comment.
Nice catch. That is a clever way to keep tabs from creeping into your code base. Confirming it was more correct in the original. There were a couple of bug fixes since ancient times, I'll check whether they need forwardly porting.
There was a problem hiding this comment.
@smarter at least I can confirm that in scala2 it doesn't affect semanticdb generation as this method isn't used in plugin.
Even without braces as in Scala2 I wouldn't say that it's defined correctly.
For example:
// scala 2.13.8
object A {
// indented with one tab
def foo: Unit = ???
def foo: Unit = ???
^^^^
error: method foo is defined twice;
the conflicting method foo was defined at line 6:13
}Notice that position in message is 6:13 while it should be 6:7
There was a problem hiding this comment.
Where does the example output come from? Column != position. In Scala 2, leftmost column is column 1, in Scala 3 it is zero. So foo is in the 13th column. In this rendering, there is also an extra caret. What is 7?
There was a problem hiding this comment.
@som-snytt it comes from compiler error. I don't think that there is any meaningful application of having tab-to-space transformed value here instead of position.
I took a look how this method is used in scala2 and scala3 and found that is used only in a meaning of position.
Does any body knows in which case it might be used not as a position?
So here are two options:
- drop tab-to-space translation logic as in this PR
- fix this method and add one another one and replace its usage across other places (semanticdb, error messages).
@smarter wdyt?
There was a problem hiding this comment.
I assume that when a line with an error contains tabs, these things are true in Scala 2:
- The error caret is displayed at the correct position when running the scalac shell script directly
- The error caret is displayed at the correct position when running the compiler via sbt
- The error squiggly line is displayed at the correct position in Metals
Are these things also true for Scala 3 with your patch?
There was a problem hiding this comment.
the problem with tab-to-space conversion is, you either have to know the width of a tab, or you have to assume one which will be wrong in many cases. i've seen scala code using 2, 3, 4 and 8 spaces wide tabs in the wild. imho the only halfway sane thing is to have character indices in the columns information, and only assume a certain width where it's necessary to render a caret in an error message.
There was a problem hiding this comment.
I see MessageRendering.errorMsg uses startColumnPadding which is tab-preserving but then uses length to determine a count of spaces.
(Sorry, I didn't realize we're talking about post-processing the tabs-translated message.)
In Scala 2, source had a notion of underlying source for scripts and any file where positions are translated.
The problem of rendered char column vs index of code point is the same for Unicode.
I agree that all this is a rendering concern. For example, the ambitious startColumnPadding is just the slice from startOfLine to the offset, which can be rendered any way you like.
There was a problem hiding this comment.
@smarter yep, this change doesn't affect the things you mentioned and they works fine
There was a problem hiding this comment.
ok, then I guess this change is fine and I'll let others decide if more needs to be done.
|
I was curious about anything worth forward porting, so I #15224 One functional change was that Scala 2 always ensured content ended in a newline, Scala 3 does not. |
|
When column positioning gets a bad rap, or rep, it's called columny instead of calumny. |
In case if code is indented with tabs `column` was always retuning `0`. Originally reported in scalamete/metals#3724
dos65
force-pushed
the
fix_positions_for_tab_indented
branch
from
e4be70d to
5cb1abf
Compare
| @@ -197,12 +194,7 @@ class SourceFile(val file: AbstractFile, computeContent: => Array[Char]) extends | |||
| /** The column corresponding to `offset`, starting at 0 */ | |||
| def column(offset: Int): Int = { | |||
| var idx = startOfLine(offset) | |||
| var col = 0 | |||
| while (idx != offset) { | |||
| col += (if (idx < content().length && content()(idx) == '\t') (tabInc - col) % tabInc else 1) | |||
There was a problem hiding this comment.
ok, then I guess this change is fine and I'll let others decide if more needs to be done.
In case if code is indented with tabs
columnwas always retuning0.Originally reported in scalameta/metals#3724