Added logic to avoid losing range information#771
Added logic to avoid losing range information#771lihaoyi merged 3 commits intocom-lihaoyi:masterfrom
Conversation
* Adds logic to shift the point in range positions (it does not shift start / end because of some internal validation) * Shares the source file trimming between nodes for optimisation
| import scala.reflect.internal.util._ | ||
|
|
||
| private val trimmedSource = new BatchSourceFile(g.currentSource.file, | ||
| g.currentSource.content.drop(topWrapperLen)) |
There was a problem hiding this comment.
this is an optimisation to avoid re-trimming the source for every node on the tree
Baccata
changed the title
Baccata
changed the title
| new TransparentPosition(trimmedSource, s.start, s.point - topWrapperLen, s.end) | ||
| case s: RangePosition if s.start > topWrapperLen => | ||
| new RangePosition(trimmedSource, s.start, s.point - topWrapperLen, s.end) | ||
| case s: OffsetPosition if s.start > topWrapperLen => |
There was a problem hiding this comment.
s.start == s.point in this case
There was a problem hiding this comment.
Shouldn't the startIn of the RangePosition also be s.start - topWrapperLen? and the endIn be s.end - topWrapperLen?
There was a problem hiding this comment.
Ideally it would be the case, but the compiler performs some later validation on the trees to make sure the ranges aren't overlapping, which throws when I try what your suggest. I'm not quite sure why, it could have to do with multi-phase scripts.
With the current change, the behaviour of ammonite remains the same whilst allowing external tools (semanticdb) to work around this issue by adding post-processing logic that would look up the position of the symbol matching the filename (thanks for that btw 😄) and applying the appropriate shift (outside of the scalac's kingdome)
I can open a ticket for further investigation and reference it in a TODO in this piece of code, if that is acceptable
There was a problem hiding this comment.
There is a ticket to tone down position validation errors in the compiler scala/scala-dev#390 The validation is required for Scala IDE to efficiently find enclosing trees given a position. semanticdb does not require the validated properties to hold so the errors are unnecessary.
There was a problem hiding this comment.
Sure that sounds good to me; @Baccata if you could put your explanation here as a comment in the code that's all I ask before merging
|
Not sure I'll ever finish shaving the yak, but my hope is to get ammonite and semanticdb-scalac compatible with each other, eventually allowing some tools written for standard Scala to work with ammonite scripts (maybe even LSP support while we're at it ^^) |
Added tests to check that the ammonite plugin shifting the point in RangePosition (rather than upcasting them to OffsetPositions) does not temper with the behaviour of Ammonite
Added a comment to explain why `start` and `end` values aren't shifted in Range positions. com-lihaoyi#771 (comment)
Added a comment to explain why `start` and `end` values aren't shifted in Range positions. com-lihaoyi#771 (comment)
Baccata
force-pushed
the
transform-range-positions
branch
from
af2c9e0 to
fb8ed68
Compare
|
Looks good to me, thanks! |
3 participants
Rationale :
When compiling ammonite script with
YrangePosenabled, the transformation performed in the AmmonitePlugin was discarding the "range" nature of the position. This allows to conserve range information, which improves interoperability with some compiler plugins.