Skip to content

Fix SourceFile.column method. #15209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 23, 2022
Merged

Fix SourceFile.column method. #15209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions compiler/src/dotty/tools/dotc/util/SourceFile.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ class SourceFile(val file: AbstractFile, computeContent: => Array[Char]) extends

def maybeIncomplete: Boolean = _maybeInComplete

/** Tab increment; can be overridden */
def tabInc: Int = 8

override def name: String = file.name
override def path: String = file.path
override def jfile: Optional[JFile] = Optional.ofNullable(file.file)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarter yep, this change doesn't affect the things you mentioned and they works fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then I guess this change is fine and I'll let others decide if more needs to be done.

idx += 1
}
col
offset - idx
}

/** The padding of the column corresponding to `offset`, includes tabs */
Expand Down
40 changes: 20 additions & 20 deletions tests/neg/doubleDefinition.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:14:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:14:5 ----------------------------------------------------------
14 | def foo(x: List[B]): Function1[B, B] = ??? // error: same jvm signature
| ^
| Double definition:
Expand All @@ -8,38 +8,38 @@
|
| Consider adding a @targetName annotation to one of the conflicting definitions
| for disambiguation.
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:21:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:21:5 ----------------------------------------------------------
21 | def foo(x: List[A]): Function2[B, B, B] = ??? // error
| ^
| Double definition:
| def foo(x: List[A]): A => A in class Test3 at line 20 and
| def foo(x: List[A]): (B, B) => B in class Test3 at line 21
| have matching parameter types.
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:26:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:26:5 ----------------------------------------------------------
26 | def foo = 2 // error
| ^
| Double definition:
| val foo: Int in class Test4 at line 25 and
| def foo: Int in class Test4 at line 26
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:31:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:31:5 ----------------------------------------------------------
31 | val foo = 1 // error
| ^
| Double definition:
| def foo: Int in class Test4b at line 30 and
| val foo: Int in class Test4b at line 31
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:36:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:36:5 ----------------------------------------------------------
36 | var foo = 1 // error
| ^
| Double definition:
| def foo: Int in class Test4c at line 35 and
| var foo: Int in class Test4c at line 36
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:41:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:41:5 ----------------------------------------------------------
41 | def foo = 2 // error
| ^
| Double definition:
| var foo: Int in class Test4d at line 40 and
| def foo: Int in class Test4d at line 41
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:55:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:55:5 ----------------------------------------------------------
55 | def foo(x: List[B]): Function1[B, B] = ??? // error: same jvm signature
| ^
| Double definition:
Expand All @@ -49,80 +49,80 @@
|
| Consider adding a @targetName annotation to one of the conflicting definitions
| for disambiguation.
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:62:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:62:5 ----------------------------------------------------------
62 | def foo(x: List[A]): Function2[B, B, B] = ??? // error
| ^
| Double definition:
| def foo(x: List[A]): A => A in trait Test7 at line 61 and
| def foo(x: List[A]): (B, B) => B in trait Test7 at line 62
| have matching parameter types.
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:67:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:67:5 ----------------------------------------------------------
67 | def foo = 2 // error
| ^
| Double definition:
| val foo: Int in class Test8 at line 66 and
| def foo: Int in class Test8 at line 67
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:72:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:72:5 ----------------------------------------------------------
72 | val foo = 1 // error
| ^
| Double definition:
| def foo: Int in class Test8b at line 71 and
| val foo: Int in class Test8b at line 72
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:77:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:77:5 ----------------------------------------------------------
77 | var foo = 1 // error
| ^
| Double definition:
| def foo: Int in class Test8c at line 76 and
| var foo: Int in class Test8c at line 77
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:82:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:82:5 ----------------------------------------------------------
82 | def foo = 2 // error
| ^
| Double definition:
| var foo: Int in class Test8d at line 81 and
| def foo: Int in class Test8d at line 82
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:88:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:88:5 ----------------------------------------------------------
88 | def foo: String // error
| ^
| Double definition:
| val foo: Int in class Test9 at line 87 and
| def foo: String in class Test9 at line 88
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:92:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:92:5 ----------------------------------------------------------
92 | def foo: Int // error
| ^
| Double definition:
| val foo: Int in class Test10 at line 91 and
| def foo: Int in class Test10 at line 92
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:96:4 ----------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:96:5 ----------------------------------------------------------
96 | def foo: String // error
| ^
| Double definition:
| val foo: Int in class Test11 at line 95 and
| def foo: String in class Test11 at line 96
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:100:4 ---------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:100:5 ---------------------------------------------------------
100 | def foo: Int // error
| ^
| Double definition:
| val foo: Int in class Test12 at line 99 and
| def foo: Int in class Test12 at line 100
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:104:4 ---------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:104:5 ---------------------------------------------------------
104 | def foo: String // error
| ^
| Double definition:
| var foo: Int in class Test13 at line 103 and
| def foo: String in class Test13 at line 104
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:108:4 ---------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:108:5 ---------------------------------------------------------
108 | def foo: Int // error
| ^
| Double definition:
| var foo: Int in class Test14 at line 107 and
| def foo: Int in class Test14 at line 108
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:112:4 ---------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:112:5 ---------------------------------------------------------
112 | def foo: String // error
| ^
| Double definition:
| var foo: Int in class Test15 at line 111 and
| def foo: String in class Test15 at line 112
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:116:4 ---------------------------------------------------------
-- [E120] Naming Error: tests/neg/doubleDefinition.scala:116:5 ---------------------------------------------------------
116 | def foo: Int // error
| ^
| Double definition:
Expand Down
8 changes: 4 additions & 4 deletions tests/neg/spaces-vs-tabs.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
| Incompatible combinations of tabs and spaces in indentation prefixes.
| Previous indent : 2 tabs
| Latest indent : 4 spaces
-- Error: tests/neg/spaces-vs-tabs.scala:6:8 ---------------------------------------------------------------------------
-- Error: tests/neg/spaces-vs-tabs.scala:6:3 ---------------------------------------------------------------------------
6 | println(3) // error
| ^
| Incompatible combinations of tabs and spaces in indentation prefixes.
| Previous indent : 2 tabs
| Latest indent : 2 spaces, 1 tab
-- Error: tests/neg/spaces-vs-tabs.scala:7:2 ---------------------------------------------------------------------------
-- Error: tests/neg/spaces-vs-tabs.scala:7:3 ---------------------------------------------------------------------------
7 | println(4) // error
| ^
| Incompatible combinations of tabs and spaces in indentation prefixes.
Expand All @@ -22,13 +22,13 @@
| Incompatible combinations of tabs and spaces in indentation prefixes.
| Previous indent : 2 tabs
| Latest indent : 1 space
-- Error: tests/neg/spaces-vs-tabs.scala:14:2 --------------------------------------------------------------------------
-- Error: tests/neg/spaces-vs-tabs.scala:14:3 --------------------------------------------------------------------------
14 | else 2 // error
| ^
| The start of this line does not match any of the previous indentation widths.
| Indentation width of current line : 1 tab, 2 spaces
| This falls between previous widths: 1 tab and 1 tab, 4 spaces
-- [E129] Potential Issue Warning: tests/neg/spaces-vs-tabs.scala:13:6 -------------------------------------------------
-- [E129] Potential Issue Warning: tests/neg/spaces-vs-tabs.scala:13:7 -------------------------------------------------
13 | 1
| ^
| A pure expression does nothing in statement position; you may be omitting necessary parentheses
Expand Down
5 changes: 5 additions & 0 deletions tests/semanticdb/expect/Tabs.expect.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package example

val a/*<-example::Tabs$package.a.*/ =
List/*->scala::package.List.*/(1,2,3)
.map/*->scala::collection::immutable::List#map().*/(_ +/*->scala::Int#`+`(+4).*/ 1)
5 changes: 5 additions & 0 deletions tests/semanticdb/expect/Tabs.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package example

val a =
List(1,2,3)
.map(_ + 1)
28 changes: 28 additions & 0 deletions tests/semanticdb/metac.expect
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3393,6 +3393,34 @@ Synthetics:
[55:6..55:12):foo(x) => *(x)
[58:6..58:12):foo(0) => *(given_Int)

expect/Tabs.scala
-----------------

Summary:
Schema => SemanticDB v4
Uri => Tabs.scala
Text => empty
Language => Scala
Symbols => 2 entries
Occurrences => 5 entries
Synthetics => 2 entries

Symbols:
example/Tabs$package. => final package object example extends Object { self: example.type => +2 decls }
example/Tabs$package.a. => val method a List[Int]

Occurrences:
[0:8..0:15): example <- example/
[2:4..2:5): a <- example/Tabs$package.a.
[3:1..3:5): List -> scala/package.List.
[4:3..4:6): map -> scala/collection/immutable/List#map().
[4:9..4:10): + -> scala/Int#`+`(+4).

Synthetics:
[3:1..4:6):List(1,2,3)
.map => *[Int]
[3:1..3:5):List => *.apply[Int]

expect/Traits.scala
-------------------

Expand Down