Skip to content
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

Enable range positions (-Yrangepos) by default #9146

Merged
merged 1 commit into from Sep 9, 2020
Merged

Enable range positions (-Yrangepos) by default #9146

merged 1 commit into from Sep 9, 2020

Conversation

eed3si9n
Copy link
Member

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

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
Member

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

@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

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

run 3638 was green

@eed3si9n
Copy link
Member Author

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

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
@@ -1,63 +1,63 @@
[[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

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

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

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
5 participants
@eed3si9n @hrhino @SethTisue @dwijnand @scala-jenkins