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
Enable range positions (-Yrangepos) by default #472
Comments
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 |
I'm looking into taking this one on, if there's no plans from scala core folks. |
Great, thank you!
…On Tue, Feb 13, 2018 at 19:13 Harrison Houghton ***@***.***> 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
<#472 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFjy02k-6YnnEdKuV-YYSDsRzzdg4Oeks5tUdDNgaJpZM4SEKSQ>
.
|
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 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,
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 I tend to think we should move positions into one or two primitive fields directly on |
The motivation is that the validation step isn't fast, and takes up a good chunk of the "rangepos penalty" time difference. Moreover, Alex Average User can't do much about a fatal rangepos error other than twiddle around their source until it goes away, so it's likely to bother end users less like this. However, since positions are important, turn the flag on unconditionally for partesting. References scala/scala-dev#472.
The motivation is that the validation step isn't fast, and takes up a good chunk of the "rangepos penalty" time difference. Moreover, Alex Average User can't do much about a fatal rangepos error other than twiddle around their source until it goes away, so it's likely to bother end users less like this. However, since positions are important, turn the flag on unconditionally for partesting. References scala/scala-dev#472.
The motivation is that the validation step isn't fast, and takes up a good chunk of the "rangepos penalty" time difference. Moreover, Alex Average User can't do much about a fatal rangepos error other than twiddle around their source until it goes away, so it's likely to bother end users less like this. This is a backport to 2.12.x, since position validation is changing for performance, and we evidently want to be cautious about adding new breakages. References scala/scala-dev#472.
The motivation is that the validation step isn't fast, and takes up a good chunk of the "rangepos penalty" time difference. Moreover, Alex Average User can't do much about a fatal rangepos error other than twiddle around their source until it goes away, so it's likely to bother end users less like this. This is a backport to 2.12.x, since position validation is changing for performance, and we evidently want to be cautious about adding new breakages. References scala/scala-dev#472.
this won't make M5. could it still make RC1? |
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 ThisBuild / semanticdbEnabled := true setting. Default is If |
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 |
I happened upon this the other day, in our
I don't know anything about (#487 seems related, but doesn't answer my question.) |
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. |
/cc @hrhino |
|
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 #430 (comment) @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:
The text was updated successfully, but these errors were encountered: