Scala-dev: Enable range positions (-Yrangepos) by default

[SethTisue]

this isn't a new idea, but it hasn't had its own ticket before, at least, I can't find one.

at the umbrella ticket https://github.com/scala/scala-dev/issues/430#issuecomment-332562635 @adriaanm said (in response to @xeno-by's question) rangepos "will be on by default" in 2.13, as a goal at least

@olafurpg has said "The Scalameta Semantic API, which Scalafix uses, requires -Yrangepos" (here) so this is an important area for anyone interested in improving the quality of tooling for Scala

some relevant links:

• https://github.com/scala/bug/labels/positions
• https://github.com/scala/scala-dev/issues/390

[SethTisue]

this is a good one for anyone interested in helping out with compiler/tooling stuff, since it's partly just a matter of fixing issues the with range positions one at a time

[hrhino]

I'm looking into taking this one on, if there's no plans from scala core folks.

[adriaanm]

Great, thank you!
On Tue, Feb 13, 2018 at 19:13 Harrison Houghton [e-mail]
wrote:

I'm looking into taking this one on, if there's no plans from scala core
folks.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/scala/scala-dev/issues/472#issuecomment-365354687,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFjy02k-6YnnEdKuV-YYSDsRzzdg4Oeks5tUdDNgaJpZM4SEKSQ
.

[retronym]

The range position invariants, enforced with unforgiving assertions, are all about making sure that the presentation compiler is able to locate a position with the AST by navigating down the child tree (of, say, a Block) that encloses it. If two child trees were allowed to enclose the same position, the search would need to try both.

Rewrites like names/defaults, eta-expansion end up with positions that are awkward to fit into that clean model, and have to use transparent positions to guide the search for the smallest enclosing tree or context

Relatedly, atPos adds cleans up positions of trees of synthesized blocks of code to maintain the invariants, which it usefully documents as::

/** Handling range positions
 *  atPos, the main method in this trait, will add positions to a tree,
 *  and will ensure the following properties:
 *
 *    1. All nodes between the root of the tree and nodes that already have positions
 *       will be assigned positions.
 *    2. No node which already has a position will be assigned a different range; however
 *       a RangePosition might become a TransparentPosition.
 *    3. The position of each assigned node includes the positions of each of its children.
 *    4. The positions of all solid descendants of children of an assigned node
 *       are mutually non-overlapping.
 *
 * Here, the solid descendant of a node are:
 *
 *   If the node has a TransparentPosition, the solid descendants of all its children
 *   Otherwise, the singleton consisting of the node itself.
 */

Why are violations of these invariants were reported as hard-error, rather than treating them as bugs that would just degrade the IDE functionality? I understand it was to make the compiler team a bit more responsive to the needs of the IDE team.

390 suggests to tone down the warning, at least for the use case of enabling range positions in the batch compiler.

We used to have a nightly build that ran partest with range positions enabled. I'd suggest that you submit a PR to enabled rangepos by default to see where we stand with regards to failing tests. I can also give it a spin through the benchmarks to see if we still see a performance drop.

I tend to think we should move positions into one or two primitive fields directly on Tree rather than sharing the attachments field. Dotty packs start/end/focus into a Long.

[SethTisue]

this won't make M5. could it still make RC1?

[eed3si9n]

Both -Yrangepos and SemanticDB are needed to get full Metals feature, so as more people try Metals, those things will be turned on. In my interest from the build angle, I'd like all builds to be repeatable, so I'm in favor of encoding changes into build.sbt file, esp if there's a risk of changing the semantics. This led me to add:

ThisBuild / semanticdbEnabled := true

setting. Default is false in sbt 1.3.x.

If -Yrangepos is enabled across the board, then there would be less variance between the build flavors (CI builds, Community Builds, Metals-ready builds, etc), so I think that would be better in the same vein that we would want fewer -Y options in general. For LSP, range position is the future.

[SethTisue]

I asked around on our team about whether we could/should target this for an upcoming 2.13.x release.

@lrytz was in favor.

Concerns on the team included safety and performance.

Safety

Metals users have it enabled, which means the flag has been getting a good workout on a variety of codebases, as Metals popularity grows. Also, at least one customer with an extremely large Scala codebase has it enabled;

The Scala community build could be of assistance here. We could enable the flag build-wide as a trial and see what problems turn up, if any.

Performance

says @retronym, "I've been chipping away at the the performance impacts of having it enabled." He tried it in compiler-benchmark and the results suggest "performance is very close. About 1.5% more allocation for range positions, actual compile times are the same within margin for error"

me (Seth): "I think it's worth the at-most-1.5%", Dale concurred

[SethTisue]

I happened upon this the other day, in our build.sbt:

    // enable this in 2.13, when tests pass
    //scalacOptions in Compile += "-Yvalidate-pos:parser,typer",

I don't know anything about -Yvalidate-pos. Is there any action needed here, or should I just delete the commented-out lines?

(#487 seems related, but doesn't answer my question.)

[retronym]

The compiler used to always run this validation and fail compilation when an range position invariant was broken (ie. the compiler had a bug that wasn't triggered in our own test suite but only happened in the user's code).

scala/scala#6754 made this hard-failure opt-in, both the save the cost of the analysis and blaming users for compiler bugs. The impact of a broken range position invariant is typically a failure in the presentation compiler (Scala IDE or the REPL) to locate the AST at the cursor to perform completions, type hints, etc).

I believe the intent of the comment was that it should have been uncommented when merging forward to 2.13.x. That seems worth trying now.

[retronym]

/cc @hrhino

[dwijnand]

I believe the intent of the comment was that it should have been uncommented when merging forward to 2.13.x. That seems worth trying now.

https://github.com/scala/scala/pull/9338