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

Source locators are incorrect in error messages #2681

Closed
jackkoenig opened this issue Aug 3, 2023 · 2 comments · Fixed by #2686
Closed

Source locators are incorrect in error messages #2681

jackkoenig opened this issue Aug 3, 2023 · 2 comments · Fixed by #2686
Milestone

Comments

@jackkoenig
Copy link

Consider the following:

$ mill --version
Mill Build Tool version 0.11.1
Java version: 11.0.13, vendor: GraalVM Community, runtime: /Library/Java/JavaVirtualMachines/openjdk11-graalvm/Contents/Home
Default locale: en_US, platform encoding: UTF-8
OS name: "Mac OS X", version: 12.6.2, arch: x86_64

$ cat build.sc
// A comment
import mill._, scalalib._, publish._

object proj extends JavaModule {
  def foo = T { os.pwd / 3 }
}

$ mill show proj.foo
[build.sc] [41/49] compile 
[info] compiling 1 Scala source to /Users/jackk/work/foo/out/mill-build/compile.dest/classes ...
[error] /Users/jackk/work/foo/build.sc:4:26: type mismatch;
[error]  found   : Int(3)
[error]  required: os.PathChunk
[error]   def foo = T { os.pwd / 3 }
[error]                          ^
[error] one error found
1 targets failed
compile Compilation failed

The line number given is 4, but it should be 5. I am pretty sure this offset is due to the comment above the imports because if I add more lines before the imports, the offset increases proportionally to the number of lines I've added.

@lihaoyi
Copy link
Member

lihaoyi commented Aug 5, 2023

Seems like the same issue as com-lihaoyi/Ammonite#1361, which was fixed upstream in Ammonite after we vendored the code in Mill. We should pull in the latest fixed code from Ammonite (including that PR and some follow ups)

lihaoyi added a commit that referenced this issue Aug 8, 2023
… or whitespace (#2686)

This is basically a port of the following fix in the Ammonite repo
com-lihaoyi/Ammonite#1361. There were some
changes when the logic was first ported from Ammonite to Mill, resulting
in a much smaller code change than was necessary in the Ammonite repo

There are two related follow up PRs that AFAICT do not apply to Mill:
com-lihaoyi/Ammonite#1363
and com-lihaoyi/Ammonite#1369. Both have to do
with the special handling of multi-statement top-level-blocks `{...}`,
that is a thing in the Ammonite REPL (to allow multiple top-level
declarations to be entered into the REPL before submitting it) but
doesn't apply to Mill scripts.

I have updated the integration tests to include a bunch of random
comments and imports to reproduce the problem causing the tests to fail,
and verified the fix causes the tests to pass. Also this caused a
previously passing test to fail (`2-custom-build-logic`), which on
inspection appears like it was asserting the wrong thing, so I fixed the
assert

Fixes #2681
@jackkoenig
Copy link
Author

Thanks for the fix!

@lefou lefou added this to the 0.11.2 milestone Aug 9, 2023
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 a pull request may close this issue.

3 participants