sbt.internal.inc.Position: add startOffset and endOffset #571

smarter · 2018-08-12T16:37:05Z

This is a follow-up to sbt/util#173

smarter · 2018-08-12T16:37:49Z

(Running the validation against sbt/util#173)

typesafe-tools · 2018-08-12T18:11:29Z

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project	Reference	Commit
sbt	1.2.x	sbt/sbt@`549c694`
zinc	pull/571/head	`6bd672b`
io	1.2.x	sbt/io@`fb0acb1`
librarymanagement	1.2.x	sbt/librarymanagement@`f42ec6f`
util	pull/173/head	sbt/util@`1b2f2cd`
website	1.2.x

✅ The result is: SUCCESS
(restart)

jvican

Let's not upgrade to sbt 1.2.1 here, since it causes build problems (last time I tried). Alternatively, I guess the build also fails here because we have yet to publish a jar with sbt/util#173 (review). Otherwise the patch LGTM!

A position now has a start, an end, and a point (the existing `offset`), just like it does in the Scala compiler. This information is especially useful for displaying squiggly lines in an IDE. This commit and the next one are required for sbt/zinc#571

eed3si9n · 2018-08-13T22:04:02Z

So should this PR target 1.x branch?

smarter · 2018-08-14T03:47:39Z

Any reason it couldn't get in the next 1.2 release?

jvican · 2018-08-14T07:24:55Z

Targeting 1.x is better, we could branch out (or cherry pick) in 1.2.2 from there.

eed3si9n · 2018-08-14T15:31:42Z

Any reason it couldn't get in the next 1.2 release?

I think it depends on how critical this change is. For sbt, we don't go through RC cycle for patch releases, so we try to keep them to bug fixes. Considering that 1.3.0 will be a few months away, I'm open to a few enhancements added during 1.2.x but we need to be fairly confident that it won't cause compatibility problems.

smarter · 2018-08-14T15:37:23Z

It's critical to getting any BSP implementation for Scala working. My goal is for us to be able to give the Intro to FP course at EPFL that starts in mid-September using Dotty, which means using the Dotty IDE, which would really benefit from the BSP.

jvican · 2018-08-14T15:39:28Z

I second this change, compatibility issues with this patch are close to zero IMO.

Follow up to sbt/util#173

Moved to 1.x as requested, and I'll make a PR for backporting it as soon as it is merged.

smarter · 2018-08-18T07:20:48Z

internal/compiler-bridge/src/main/scala/xsbt/DelegatingReporter.scala

+
+      // Same logic as Position#line
+      def lineOf(offset: Int) = src.offsetToLine(offset) + 1
+      def columnOf(offset: Int) = offset - src.lineToOffset(src.offsetToLine(offset))


It's always been this way, but isn't it weird that lines in the zinc Reporter are 1-based and columns are 0-based ?

Yes, I would welcome a change here, I have no idea of the rationale of the previous change.

jvican

LGTM

eed3si9n

LGTM

This is a backport of sbt#571.

This is a backport of sbt#571 to the 1.2.x branch.

A position now has a start, an end, and a point (the existing `offset`), just like it does in the Scala compiler. This information is especially useful for displaying squiggly lines in an IDE. This commit and the next one are required for sbt/zinc#571

retronym · 2020-01-26T04:16:06Z

This change seems to interact poorly with Scaladoc. I think the root bug is in Scaladoc which is spitting out an invalid end position for use cases.

Minimized: https://github.com/retronym/sbt-scaladoc-aioooe

[info] Main Scala API documentation to /Users/jz/code/sbt-scaladoc-aioooe/target/scala-2.13/api...
[error] java.lang.ArrayIndexOutOfBoundsException: 14
[error] 	at scala.reflect.internal.util.BatchSourceFile.findLine$1(SourceFile.scala:200)
[error] 	at scala.reflect.internal.util.BatchSourceFile.offsetToLine(SourceFile.scala:203)
[error] 	at xsbt.DelegatingReporter$.lineOf$1(DelegatingReporter.scala:99)
[error] 	at xsbt.DelegatingReporter$.makePosition$1(DelegatingReporter.scala:112)
[error] 	at xsbt.DelegatingReporter$.convert(DelegatingReporter.scala:134)
[error] 	at xsbt.DelegatingReporter.info0(DelegatingReporter.scala:165)
[error] 	at scala.tools.nsc.reporters.MakeFilteringForwardingReporter.doReport(ForwardingReporter.scala:59)
[error] 	at scala.tools.nsc.reporters.FilteringReporter.info0(Reporter.scala:93)
[error] 	at scala.reflect.internal.Reporter.filteredInfo(Reporting.scala:118)
[error] 	at scala.reflect.internal.Reporter.warning(Reporting.scala:112)
[error] 	at scala.tools.nsc.doc.ScaladocAnalyzer$ScaladocTyper.$anonfun$typedDocDef$1(ScaladocAnalyzer.scala:47)
[error] 	at scala.tools.nsc.doc.ScaladocAnalyzer$ScaladocTyper.typedDocDef(ScaladocAnalyzer.scala:44)
[error] 	at scala.tools.nsc.doc.ScaladocAnalyzer$ScaladocTyper.typedDocDef$(ScaladocAnalyzer.scala:36)

Zinc could guard against the dodgy positions here. I'll see if I can figure out how to fix the positions in Scaladoc itself.

retronym · 2020-01-28T00:39:31Z

Submitted a Scala bug for this: scala/bug#11865

eed3si9n · 2020-01-28T21:52:43Z

Thanks @retronym. Opened #734 for Zinc

typesafe-tools mentioned this pull request Aug 12, 2018

xsbti.Position: add startOffset and endOffset sbt/util#173

Merged

jvican previously requested changes Aug 12, 2018

View reviewed changes

jvican added community binary api feature labels Aug 12, 2018

smarter force-pushed the position-range branch 3 times, most recently from 82b57d6 to 9d9741f Compare August 18, 2018 06:19

smarter changed the base branch from 1.2.x to 1.x August 18, 2018 06:19

sbt.internal.inc.Position: record range information

ed1b515

Follow up to sbt/util#173

smarter force-pushed the position-range branch from 9d9741f to ed1b515 Compare August 18, 2018 06:26

smarter requested review from eed3si9n and jvican August 18, 2018 07:16

smarter commented Aug 18, 2018

View reviewed changes

jvican approved these changes Aug 18, 2018

View reviewed changes

eed3si9n approved these changes Aug 22, 2018

View reviewed changes

eed3si9n merged commit 88e0827 into sbt:1.x Aug 22, 2018

smarter added a commit to smarter/zinc that referenced this pull request Aug 22, 2018

[Backport] sbt.internal.inc.Position: record range information

a92efd8

This is a backport of sbt#571.

smarter added a commit to smarter/zinc that referenced this pull request Aug 22, 2018

[Backport] sbt.internal.inc.Position: record range information

a5a32a1

This is a backport of sbt#571 to the 1.2.x branch.

smarter mentioned this pull request Aug 22, 2018

[Backport] sbt.internal.inc.Position: record range information #580

Merged

This was referenced Jan 26, 2020

[backport] Upgrade 2.12.x build to SBT 1.3.7 [ci: last-only] scala/scala#8660

Merged

Scaladoc use case has invalid end in range position which triggers crash in recent Zinc/SBT releases scala/bug#11865

Closed

eed3si9n mentioned this pull request Jan 28, 2020

Scaladoc may generate bad position #734

Closed

sbt.internal.inc.Position: add startOffset and endOffset #571

sbt.internal.inc.Position: add startOffset and endOffset #571

Uh oh!

Conversation

smarter commented Aug 12, 2018

Uh oh!

smarter commented Aug 12, 2018

Uh oh!

typesafe-tools commented Aug 12, 2018

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

eed3si9n commented Aug 13, 2018

Uh oh!

smarter commented Aug 14, 2018

Uh oh!

jvican commented Aug 14, 2018

Uh oh!

eed3si9n commented Aug 14, 2018

Uh oh!

smarter commented Aug 14, 2018

Uh oh!

jvican commented Aug 14, 2018

Uh oh!

smarter Aug 18, 2018

Choose a reason for hiding this comment

Uh oh!

jvican Aug 18, 2018

Choose a reason for hiding this comment

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

eed3si9n left a comment

Choose a reason for hiding this comment

Uh oh!

retronym commented Jan 26, 2020

Uh oh!

retronym commented Jan 28, 2020

Uh oh!

eed3si9n commented Jan 28, 2020

Uh oh!

Uh oh!