Skip to content

Fix toString for PositionBridge #12735

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.

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 2 commits into from
Jun 11, 2021

Conversation

mims-github
Copy link

@mims-github mims-github commented Jun 7, 2021

I migrated a small project to Scala3, using https://github.com/davidB/scala-maven-plugin.
If everything compiles fine, all good.
But if their is a compile error or warning, instead of showing the source position eg. path/file.scala:120 of the compilation problem, the following is shown instead: [ERROR] dotty.tools.xsbt.PositionBridge@767bb276: could not find Lazy implicit value of type ....
It looks like a specific toString impl is missing, and the Object.toString is used instead, which is not very helpful for the user to fix compilation problem. 😅 Check for more examples: davidB/scala-maven-plugin#477
I debugged the compilation and I think I found the place where the toString of the position is called: https://github.com/sbt/sbt/blob/develop/internal/util-logging/src/main/scala/sbt/util/InterfaceUtil.scala#L176

Even if SBT is relying on an undocumented behavior of the toString of some classes, which should probably be fixed in SBT, a more helpful toString for PositionBridge should not make any harm either.

This PR therefor adds a very basic, but helpful toString impl to PositionBridge.
Note that the other field src is also part of pos, so I skipped it in the toString, to not get the information twice.

Example call path:

log:45, SbtLogger (sbt_inc)
log:47, Logger (sbt.util)
warn:43, Logger (sbt.util)
logWarning:159, LoggedReporter (sbt.internal.inc)
display:168, LoggedReporter (sbt.internal.inc)
log:142, LoggedReporter (sbt.internal.inc)
doReport:43, DelegatingReporter (dotty.tools.xsbt)
report:150, Reporter (dotty.tools.dotc.reporting)
issueWarning:32, report$ (dotty.tools.dotc)
warning:67, report$ (dotty.tools.dotc)
checkExhaustivity:847, SpaceEngine (dotty.tools.dotc.transform.patmat)
transformMatch:45, PatternMatcher (dotty.tools.dotc.transform)

...
handleCompilationError:332, IncrementalCompilerImpl (sbt.internal.inc)
compileIncrementally:420, IncrementalCompilerImpl (sbt.internal.inc)
compile:137, IncrementalCompilerImpl (sbt.internal.inc)
compile:179, SbtIncrementalCompiler (sbt_inc)
incrementalCompile:356, ScalaCompilerSupport (scala_maven)
compile:113, ScalaCompilerSupport (scala_maven)
doExecute:88, ScalaCompilerSupport (scala_maven)
execute:308, ScalaMojoSupport (scala_maven)
executeMojo:137, DefaultBuildPluginManager (org.apache.maven.plugin)

@mims-github mims-github changed the title Fix position bridge to string Fix toString for PositionBridge Jun 7, 2021
@mims-github mims-github force-pushed the fix-PositionBridge-toString branch from 8e15720 to e6ed74c Compare June 7, 2021 15:11
There are compile call paths relying on
meaningful toString impl. for error/warning
log messages. Therefor lets add toString, which is:
"concise but informative representation that is easy for a person to read."

Without this, e.g. users of https://github.com/davidB/scala-maven-plugin
get unsuitable compile problems like:
[Warn] dotty.tools.xsbt.PositionBridge@4fbdc20: match may not be exhaustive....

Example call path:
log:45, SbtLogger (sbt_inc)
log:47, Logger (sbt.util)
warn:43, Logger (sbt.util)
logWarning:159, LoggedReporter (sbt.internal.inc)
display:168, LoggedReporter (sbt.internal.inc)
log:142, LoggedReporter (sbt.internal.inc)
doReport:43, DelegatingReporter (dotty.tools.xsbt)
report:150, Reporter (dotty.tools.dotc.reporting)
issueWarning:32, report$ (dotty.tools.dotc)
warning:67, report$ (dotty.tools.dotc)
checkExhaustivity:847, SpaceEngine (dotty.tools.dotc.transform.patmat)
....
@mims-github mims-github force-pushed the fix-PositionBridge-toString branch from e6ed74c to 8165f99 Compare June 8, 2021 05:45
@mims-github
Copy link
Author

mims-github commented Jun 10, 2021

Please let me know, if I can help to get this PR merged, as this fixes for us a blocker to upgrade to Scala3.
Thanks.
PS: So your help and fast feedback was very appreciated. 😄

Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

LGTM

@adpi2 adpi2 merged commit fcd837a into scala:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants