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

Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled [ci: last-only] #6831

Merged
merged 2 commits into from Nov 2, 2018

Conversation

Projects
None yet
6 participants
@som-snytt
Contributor

som-snytt commented Jun 20, 2018

Follows up #6559 with requiring postfix flag.

This change makes the postfixOps language feature more opt-in, to discourage newbies from exploiting the feature and falling into its snares.

The tide turned against postfix syntax on the forum.

Codebases relying on postfix syntax will want to enable -language:postfixOps.

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone Jun 20, 2018

@som-snytt som-snytt changed the title from Postfix flip switch [ci:last-only] to Postfix flip switch from warn to error [ci:last-only] Jul 17, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jul 17, 2018

needs rebase

@SethTisue SethTisue changed the title from Postfix flip switch from warn to error [ci:last-only] to Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled [ci:last-only] Jul 17, 2018

@som-snytt som-snytt force-pushed the som-snytt:issue/postfix-flip-switch branch from c6a05ba to 6b79e67 Jul 18, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 7, 2018

@som-snytt what's next here?

@SethTisue SethTisue self-assigned this Aug 7, 2018

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Aug 7, 2018

@SethTisue IIUC @adriaanm has to defer things he'd rather be doing to do some sort of restarr dance. "Dancing with the Restarrs!" Some people would love to get a shot at that.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 7, 2018

When starr becomes M5, we can rebase and merge this for RC1. Right?

@SethTisue SethTisue modified the milestones: 2.13.0-M5, 2.13.0-RC1 Aug 7, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 31, 2018

@som-snytt when you wish upon a STARR you wake up and M5 is far behind you

@SethTisue SethTisue removed their assignment Aug 31, 2018

@adriaanm adriaanm force-pushed the som-snytt:issue/postfix-flip-switch branch from 6b79e67 to afd25cb Aug 31, 2018

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 31, 2018

🕺 🌠

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 31, 2018

compilation errors when building partest

@som-snytt som-snytt force-pushed the som-snytt:issue/postfix-flip-switch branch from afd25cb to 2f6223a Aug 31, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Aug 31, 2018

[test] [error]   - library/compile:compileIncremental failed: Error compiling sbt component 'compiler-interface'

looks like this needs the special checkGoldenUnits dispensation?

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Aug 31, 2018

That should be in starr.

Maybe the update includes an sbt change. The test requires the xsbt file be compiled by itself.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Aug 31, 2018

Indeed, debug says

xsbt compat compiled with 15 units!!!

I probably won't bother checking if the behavior changed; the test for one unit was just for insurance, since the test only checks trailing characters in the path. Oh wait, it was also nice not to check a possibly long list of units for the special name.

I wonder if @dwijnand knows if sbt changed how it compiles xsbt/Compat.scala. Edit: the last bump was to 0.13.17, months ago.

Looking at the kludge now, I see it sets the language option, but doesn't restore it. Another fix would have been to inject a root import of the language feature, now that I know how root imports work. One could also imagine encapsulating the special knowledge about the path in SourceFile.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Aug 31, 2018

Well there's sbt/zinc@c60ff23, but that's not that recent.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Aug 31, 2018

@dwijnand Thanks. My issue is that I got the idea that the Compat.scala files is always compiled by itself, so I checked that first for purposes of ignoring postfix in that file. But now 15 files are compiled with it. (I'm about to check which.)

@@ -1,13 +1,11 @@
/* __ *\
** ________ ___ / / ___ Scala API **
** / __/ __// _ | / / / _ | (c) 2003-2015, LAMP/EPFL **
** / __/ __// _ | / / / _ | (c) 2003-2018, LAMP/EPFL **

This comment has been minimized.

@som-snytt

som-snytt Oct 6, 2018

Contributor

What could it hurt? he said.

@som-snytt som-snytt changed the title from Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled [ci:last-only] to Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled [ci: last-only] Oct 6, 2018

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Oct 6, 2018

Under 0.13.16. (The fix is in 0.13.18, sbt/sbt#4132.)

[info] 'compiler-interface' not yet compiled for Scala (unknown). Compiling...
Compiling 15 units: List(API.scala, Analyzer.scala, Command.scala, Compat.scala, CompilerInterface.scala, ConsoleInterface.scala, DelegatingReporter.scala, Dependency.scala, ExtractAPI.scala, ExtractUsedNames.scala, GlobalHelpers.scala, LocateClassFile.scala, Log.scala, Message.scala, ScaladocInterface.scala)
/tmp/sbt_3bc84e4d/xsbt/Compat.scala:152: error: postfix operator headOption needs to be enabled
by making the implicit value scala.language.postfixOps visible.
This can be achieved by adding the import clause 'import scala.language.postfixOps'
or by setting the compiler option -language:postfixOps.
See the Scaladoc for value scala.language.postfixOps for a discussion
why the feature needs to be explicitly enabled.
          } headOption
            ^
one error found
@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Oct 6, 2018

The compat kludge is improved in #7319

@som-snytt som-snytt force-pushed the som-snytt:issue/postfix-flip-switch branch from 2f6223a to 55f2851 Oct 6, 2018

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Oct 8, 2018

There ought to be a scalafix for:

for (e <- it(coll)) f(e) foreach (b +=)
@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Oct 8, 2018

That "double-feature" test has a clever name, but it relies on postfix to trigger, probably because typechecking happens twice but not silently. But it won't be reported twice as an error. Ironic! as Alanis would say. The test should be changed to just report twice manually.

Require postfixOps feature setting
This commit turns the feature warning into an error
if `postfixOps` is not enabled.

Popular sentiment has turned against the feature,
and requiring enablement raises the barrier to entry.

In particular, beginners will not stumble across
it accidentally.

@som-snytt som-snytt force-pushed the som-snytt:issue/postfix-flip-switch branch from 55f2851 to 3e0d8cb Oct 9, 2018

@lrytz

This comment has been minimized.

Member

lrytz commented Nov 2, 2018

This seems to be ready. @SethTisue merge?

@SethTisue

This comment has been minimized.

Member

SethTisue commented Nov 2, 2018

I need to brace myself for the likely avalanche of needed project tweaks in the community build first :-)

@lrytz

This comment has been minimized.

Member

lrytz commented Nov 2, 2018

Ah, right :-)

@SethTisue SethTisue merged commit e84cfef into scala:2.13.x Nov 2, 2018

3 checks passed

cla @som-snytt signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [5054] SUCCESS. Took 31 min.
Details
@SethTisue

This comment has been minimized.

Member

SethTisue commented Nov 2, 2018

@som-snytt would you mind updating the PR description to briefly describe and justify the change, since it will be linked from the release notes?I think this change could be surprising and annoying for some users, so I think we should have some justification only one click away from the release notes. (I'm strongly in favor of the change regardless, of course. language simplification, onward!)

@SethTisue

This comment has been minimized.

Member

SethTisue commented Nov 2, 2018

@odersky when you're listing language features that are being removed, e.g. procedure syntax, you might like to include this as well

@SethTisue

This comment has been minimized.

Member

SethTisue commented Dec 6, 2018

I need to brace myself for the likely avalanche of needed project tweaks in the community build first

it has been a puny avalanche so far, only a few pebbles here and there 👍

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