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

Added logic to avoid losing range information #771

Merged
merged 3 commits into from Mar 18, 2018

Conversation

Projects
None yet
3 participants
@Baccata
Contributor

Baccata commented Mar 15, 2018

  • Adds logic to shift the point in range positions (it does not shift start / end because of some internal validation that crashes the compilation)
  • Shares the source file trimming between nodes for optimisation

Rationale :

When compiling ammonite script with YrangePos enabled, 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.

Added logic to avoid losing range information
* 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))

This comment has been minimized.

@Baccata

Baccata Mar 15, 2018

Contributor

this is an optimisation to avoid re-trimming the source for every node on the tree

@Baccata Baccata changed the title from Added logic to avoid losing range information to [WIP] Added logic to avoid losing range information Mar 15, 2018

@Baccata Baccata changed the title from [WIP] Added logic to avoid losing range information to Added logic to avoid losing range information Mar 15, 2018

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 =>

This comment has been minimized.

@Baccata

Baccata Mar 15, 2018

Contributor

s.start == s.point in this case

This comment has been minimized.

@lihaoyi

lihaoyi Mar 16, 2018

Owner

Shouldn't the startIn of the RangePosition also be s.start - topWrapperLen? and the endIn be s.end - topWrapperLen?

This comment has been minimized.

@Baccata

Baccata Mar 16, 2018

Contributor

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

This comment has been minimized.

@olafurpg

olafurpg Mar 16, 2018

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.

This comment has been minimized.

@lihaoyi

lihaoyi Mar 18, 2018

Owner

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

This comment has been minimized.

@Baccata

Baccata Mar 18, 2018

Contributor

Done ! fb8ed68

@Baccata

This comment has been minimized.

Contributor

Baccata commented Mar 15, 2018

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 ^^)

Adding Yrangepos related tests
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

Baccata added a commit to Baccata/Ammonite that referenced this pull request Mar 18, 2018

Added a comment explaining absence of shift
Added a comment to explain why `start` and `end` values aren't
shifted in Range positions.

lihaoyi#771 (comment)
Added a comment explaining absence of shift
Added a comment to explain why `start` and `end` values aren't
shifted in Range positions.

#771 (comment)
@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 18, 2018

Looks good to me, thanks!

@lihaoyi lihaoyi merged commit ac754ee into lihaoyi:master Mar 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Baccata Baccata deleted the Baccata:transform-range-positions branch Mar 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment