Skip to content

Enable range positions (-Yrangepos) by default #9146

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.

Sign up for GitHub

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

Jump to bottom
Merged
dwijnand merged 1 commit into from
Sep 9, 2020
Merged

Enable range positions (-Yrangepos) by default #9146

dwijnand merged 1 commit into from
Sep 9, 2020

Conversation

@eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Aug 1, 2020 •
edited by SethTisue
Loading

This mainly affects users of Scalameta SemanticDB, which previously required adding -Yrangepos to scalacOptions. (But continuing to supply the flag, for example when cross-building to 2.12, is harmless.)

Note that for a long time now, Metals has enabled -Yrangepos on users' behalf. So Metals users won't notice any difference, and the fact that so many users have had -Yrangepos enabled for them behind the scenes increases our confidence that the flag is robust.

Fixes scala/scala-dev#472

@scala-jenkins scala-jenkins added this to the 2.13.4 milestone Aug 1, 2020
@hrhino
Copy link
Contributor

hrhino commented Aug 2, 2020

Wait, this is reflect.runtime.Settings... I think you (also) need to change the version in scala.tools.nsc.settings.Settings for the compiler flag.

@eed3si9n
Copy link
Member Author

eed3si9n commented Aug 2, 2020

Yea. That makes sense!

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Aug 3, 2020
@SethTisue SethTisue marked this pull request as draft August 3, 2020 18:51
@eed3si9n
Copy link
Member Author

eed3si9n commented Aug 4, 2020 

!!    1 - run/StubErrorTypeDef.scala                [output differs]
!!    2 - run/macro-rangepos-args                   [output differs]
!!    3 - run/sd187.scala                           [output differs]
!!    4 - run/string-switch-pos.scala               [output differs]
!!    5 - run/t6288.scala                           [output differs]
!!    6 - run/t7271.scala                           [output differs]
!!    8 - run/t7331a.scala                          [output differs]
!!    7 - run/t7331c.scala                          [output differs]
!!   10 - run/typetags_without_scala_reflect_typetag_lookup.scala  [output differs]
!!    9 - run/typetags_without_scala_reflect_typetag_manifest_interop.scala  [output differs]

@SethTisue
Copy link
Member

SethTisue commented Aug 4, 2020

@eed3si9n is this ready for a trip through the community build?

@eed3si9n
Copy link
Member Author

eed3si9n commented Aug 4, 2020

I think so.

@SethTisue
Copy link
Member

SethTisue commented Aug 4, 2020 •
edited
Loading

queued: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3624/
queued: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3625/
queued: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3638/

@SethTisue SethTisue changed the title Enable -Yrangepos by default Enable range positions (-Yrangepos) by default Aug 15, 2020
@SethTisue
Copy link
Member

SethTisue commented Aug 15, 2020

run 3638 was green

@eed3si9n
Copy link
Member Author

eed3si9n commented Aug 15, 2020

Nice!

dwijnand
dwijnand approved these changes Sep 2, 2020
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Any trailing reason this is draft?

test/files/run/macro-rangepos-args.check
@@ -1 +1 @@
Line: 3. Width: 1.
Line: 3. Width: 5.
Copy link
Member

@dwijnand dwijnand Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverts 0412783#diff-0298f348965a4dcbd2f8e4b61ae8ab22, so all good.

test/files/run/string-switch-pos.check
[[syntax trees at end of patmat]] // newSource1.scala
[6]package [6]<empty> {
[6]class Switch extends [13][187]scala.AnyRef {
[0:187]package [0:0]<empty> {
Copy link
Member

@dwijnand dwijnand Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That [0:0] looks wrong... but doesn't seem to affect anything.

@eed3si9n eed3si9n marked this pull request as ready for review September 2, 2020 15:07
@eed3si9n
Copy link
Member Author

eed3si9n commented Sep 2, 2020

Any trailing reason this is draft?

@SethTisue set to draft initially presumably because the build was still failing and it was unproven at the time. Now that it's passing on Community Build, I think it's ready for review / discussion.

@SethTisue
Copy link
Member

SethTisue commented Sep 2, 2020 •
edited
Loading

as already approved (modulo details, I mean) by @lrytz, I think we should bite the bullet on this for 2.13.4

everyone using Metals is running with range positions on, and Metals has been pretty popular for a while now, so that gives substantial additional confidence this is safe enough to run with

oh haha I already said that stuff at scala/scala-dev#472. but I still believe it :-)

also there I said "an upcoming 2.13.x release" but now we're saying 2.13.4 specifically

@dwijnand dwijnand merged commit 965ab82 into scala:2.13.x Sep 9, 2020
@eed3si9n eed3si9n deleted the wip/rangepos branch September 9, 2020 13:29
@SethTisue
Copy link
Member

SethTisue commented Sep 10, 2020

publicized: https://contributors.scala-lang.org/t/range-positions-yrangepos-will-be-on-by-default-in-2-13-4/4506

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

Reviewers

@dwijnand dwijnand dwijnand approved these changes

Assignees

No one assigned

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Milestone

2.13.4

Development

Successfully merging this pull request may close these issues.

Enable range positions (-Yrangepos) by default

5 participants

@eed3si9n @hrhino @SethTisue @dwijnand @scala-jenkins