Add +1 to zinc positions in the compiler bridge #8434
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
dottybot reviewed Mar 4, 2020
smarter requested changes Mar 4, 2020
| assert(line.isPresent() == true) | ||
| assert(line.get() == 9) | ||
| | ||
| // Assert disabled because we don't currently pass positions to sbt |
There was a problem hiding this comment.
I think that assert can be reenabled now.
There was a problem hiding this comment.
Reenabled the assert.
| } | ||
| public Optional<Integer> line() { | ||
| int line = pos.line(); | ||
| int line = pos.line() + 1; |
There was a problem hiding this comment.
Do we need to do anything for columns? Either way a test would be welcome.
There was a problem hiding this comment.
From what I've seen the columns are correct. I will add a test for that.
| Thanks @smarter ! |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
In the previous version of the compiler bridge we were adding 1 here:
https://github.com/sbt/zinc/blob/fc58a39402ca7abfda17133a5b3a43de3344ca49/internal/compiler-bridge/src/main/scala/xsbt/DelegatingReporter.scala#L99
which means Zinc expects line numbers starting at 1, while in Dotty we provided it with ones starting at 0.
This wasn't an issue when using dotc and sbt, since dotc added 1 to show the correct line number and sbt most likely used the Dotty rendered message due to:
https://github.com/sbt/zinc/pull/699/files?diff=unified&w=1
This popped up when adding some test in Bloop: tgodzik/bloop#1
Let me know if we should fix it differently. This is needed for the work I am doing on the Metals/Dotty integration.