Skip to content
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

Make error reporting match that of SBT's #53

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Conversation

jadenPete
Copy link

This PR fixes the following discrepancy with our compiler error reporting and SBT's:

Ours:

[Error] src/main/scala/higherkindness/rules_scala/workers/zinc/test/TestRunner.scala:233 type mismatch;
[Error]  found   : Seq[higherkindness.rules_scala.common.sbt_testing.TestDefinition]
[Error]  required: List[higherkindness.rules_scala.common.sbt_testing.TestDefinition]
one error found

Using SBT or scalac:

src/main/scala/higherkindness/rules_scala/workers/zinc/test/TestRunner.scala:233: error: type mismatch;
 found   : Seq[higherkindness.rules_scala.common.sbt_testing.TestDefinition]
 required: List[higherkindness.rules_scala.common.sbt_testing.TestDefinition]
          runner.execute(filteredTests, testScopeAndName.getOrElse(""), testRunnerArgs.frameworkArgs)
                         ^
1 error

The [Error] line prefix is added by us, but the detailed position information is missing.

Additionally, this PR fixes the test failures on lucid-master.

@jadenPete jadenPete requested a review from jjudd August 27, 2024 20:48
@jadenPete jadenPete self-assigned this Aug 27, 2024
def adjustStringPath(path: String) = SandboxUtil.getSandboxPath(workDir, Paths.get(path)).toString

options.flatMap {
case s"-P:semanticdb:targetroot:$path" =>
Copy link

Choose a reason for hiding this comment

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

Are there other other common compiler options that we know of which need a specific path?

This is a tricky issue because in theory there are infinite options which could do so. Even better yet they get specified by the user.

I suppose it's probably just not work thinking about for now.

Copy link
Author

Choose a reason for hiding this comment

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

This is a tricky issue because in theory there are infinite options which could do so. Even better yet they get specified by the user.

I thought of this when I made this change and did feel slimy modifying compiler options like this, but I think it's better to modify compiler options than pass SemanticDB options separately. My reasoning is that because there are likely many compiler options that reference paths and some of those compiler options may be used outside of this ruleset (by those defining Scala libraries, binaries, tests, etc..), it's much less confusing to intercept compiler options than direct users to use another set of options instead.

I could go through every compiler option that references file paths, but that list is probably endless and I figured we can expand this list as necessary.

tests/zinc/ErrorReporting.scala Show resolved Hide resolved
Jaden Peterson added 3 commits August 29, 2024 14:47
Not only does ProblemStringFormats format problems (compilation
messages) slightly differently, but it uses `xsbti.Position#pointer`
rather than `xsbti.Position#offset` as the column number.

Prior to v1.10.1 of Zinc, `sbt.internal.inc.javac.JavaPosition`, a
concrete implementation of `Position`, set `pointer` to `None`
unconditionally, meaning that `ProblemStringFormats` didn't have access
to the line number. To fix this, I upgraded our version of Zinc to
v1.10.1 from v1.10.0.
@jjudd jjudd force-pushed the zinc-logging-issues branch from 8ea3a21 to b1f7447 Compare August 29, 2024 20:52
@jjudd jjudd merged commit 3222051 into lucid-master Aug 29, 2024
1 check passed
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.

2 participants