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

Prepare to make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled #6559

Merged
merged 6 commits into from Jul 17, 2018

Conversation

Projects
None yet
7 participants
@som-snytt
Contributor

som-snytt commented Apr 25, 2018

Require postfixOps opt-in instead of just warning.

Fixes scala/scala-dev#462

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Apr 25, 2018

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Apr 26, 2018

After fixing gratuitous postfix in the compiler:

/var/folders/2_/xb149z895wb5f1y632xp2bw40000gq/T/sbt_19b03fbd/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
            ^

Maybe @dwijnand knows if it's possible to set the scalac options for this compilation. It's ironic that the compatibility layer uses an incompatible feature.

@som-snytt som-snytt force-pushed the som-snytt:issue/post-postfix branch from 0fe03f2 to 6d49689 Apr 26, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 26, 2018

Maybe @dwijnand knows if it's possible to set the scalac options for this compilation.

Nope, it's not configurable: https://github.com/sbt/zinc/blob/v1.1.5/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/AnalyzingCompiler.scala#L350

It's ironic that the compatibility layer uses an incompatible feature.

Hardly fair: you're using the legacy 0.13 version of sbt. Not a problem in sbt 1: sbt/zinc@ecf121a#diff-569dc93f697b5a143e904e517b998e1dL124.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Apr 26, 2018

Thanks for checking, @dwijnand

I think the irony is still apt, since that class is for 2.9-2.11 compatibility but doesn't negotiate SIP 18; but it wouldn't be fair to stretch the point.

I'm not sure if scalac will upgrade to 1.1.x soon or if needing a 0.13.18 for 2.13 would be OK.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 26, 2018

not sure if scalac will upgrade to 1.1.x soon

more reason to land #6256

if needing a 0.13.18 for 2.13 would be OK

quite probably not

@sjrd

This comment has been minimized.

Member

sjrd commented Apr 26, 2018

I'm not sure if scalac will upgrade to 1.1.x soon or if needing a 0.13.18 for 2.13 would be OK.

Even if scalac migrates to sbt 1.1.x, IIUC this change would still break users builds using sbt 0.13.x with Scala 2.13.0-M4+.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Apr 28, 2018

Yes, but if 0.13.18 is fixed, users can upgrade without much burden.

I'll look into whether a special dispensation is possible for sbt, which wouldn't be the first time.

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018

@som-snytt som-snytt force-pushed the som-snytt:issue/post-postfix branch 4 times, most recently from c3ae8a4 to e1d5208 May 3, 2018

@som-snytt som-snytt referenced this pull request May 10, 2018

Closed

Drop postfix operators #462

@lrytz lrytz requested a review from adriaanm May 28, 2018

@adriaanm

I support this conceptually, but would like to structure the implementation differently.

  1. first commit should just flip the bit on enableRequired
  2. dispensation for sbt should go earlier in the pipeline, so we hit that path check only once
  3. Ignore new warning in code base || Welcome to rebase central!
val nestedOwners =
featureTrait.owner.ownerChain.takeWhile(_ != languageFeatureModule.moduleClass).reverse
val featureName = (nestedOwners map (_.name + ".")).mkString + featureTrait.name
def action(): Boolean = {
def hasImport = inferImplicitByType(featureTrait.tpe, context).isSuccess
def hasOption = settings.language contains featureName
val OK = hasImport || hasOption
if (!OK) {
def dispensation = context.unit.source.file.path.endsWith("xsbt/Compat.scala")

This comment has been minimized.

@adriaanm

adriaanm Jun 1, 2018

Member

I'm worried about doing this during type checking. I would be more comfortable with enabling the postfix ops feature from the start when creating a compiler run for this file.

This comment has been minimized.

@som-snytt

som-snytt Jun 1, 2018

Contributor

Roger. Or you might say, "Rojair."

@som-snytt som-snytt force-pushed the som-snytt:issue/post-postfix branch from e1d5208 to 3412fe3 Jun 2, 2018

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 2, 2018

It works with local bootstrap, but no one has complained that feature warning can double-report because the explanatory part fools the reporter. (1 + must past twice through typer; the workaround is to cause the first feature warning with something benign like "hello, world" length.)

$ skalac -feature xsbt/Compat.scala
$ scalac -feature xsbt/Compat.scala
xsbt/Compat.scala:6: warning: postfix operator + should 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 should be explicitly enabled.
    List(1,2,3) map (1 +)
                       ^
xsbt/Compat.scala:6: warning: postfix operator + should be enabled
by making the implicit value scala.language.postfixOps visible.
    List(1,2,3) map (1 +)
                       ^
two warnings found

som-snytt added some commits Apr 26, 2018

Reduce postfix ops
Usages unguarded by language import. In the same
neighborhood, reduce infix as well.

@som-snytt som-snytt force-pushed the som-snytt:issue/post-postfix branch from 3412fe3 to d91f4a4 Jun 2, 2018

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jun 4, 2018

Golden tickets need to be handed out before checking them more vigorously: the sbt opt-out commit should go before the "more opt-in" commit. Also please split commits into boyscouting and strictly core changes, especially when we're flipping bits.

Finally, kill your darlings, as William said (Faulkner, not the man in black): "Make postFixOps more opt-in" is a bit too clever.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 4, 2018

This is branch order, but github shows date order. Maybe I broke something. The bit is flipped in its own commit, or do you mean you'd like the required test fixes to precede it?

Edit: reminder, don't forget to change title back to Post-postfix.

commit d91f4a4bfbd3a9b802ecebb9a9b6044c08eac0db (HEAD -> issue/post-postfix, origin/issue/post-postfix)
Author: Som Snytt <som.snytt@gmail.com>
Date:   Fri Jun 1 15:56:20 2018 -0700

    Make postfixOps more opt-in
    
    Require postfixOps feature opt-in instead of just warning.

commit fdcf987c246bb8db2cfc3f2db9a67cfdfe62128e
Author: Som Snytt <som.snytt@gmail.com>
Date:   Fri Jun 1 17:22:38 2018 -0700

    sbt gets a by

commit b3ca049600577a8227609e747868277708a28cd4
Author: Som Snytt <som.snytt@gmail.com>
Date:   Thu May 3 15:36:53 2018 -0700

    Reduce postfix ops in tests

commit b64946344b8b7b7171952a8beee649f84ac3f871
Author: Som Snytt <som.snytt@gmail.com>
Date:   Wed Apr 25 21:13:58 2018 -0700

    Reduce postfix ops
    
    Usages unguarded by language import. In the same
    neighborhood, reduce infix as well.
@adriaanm

This comment has been minimized.

Member

adriaanm commented Jun 4, 2018

Ah, right! GH using date order is really unfortunate. Scabot just blindly follows it. I assume there's a rebase option to fix this?

For the boy scouting, I was referring to the boolean algebra wizardry in the bit flip commit. It's not a big deal, but in general it's just nicer for reviewing to separate behavior changes from cleanups. Both are appreciated, just better separate.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 4, 2018

Note to self: t5675 is what rang a bell for the double-count issue for -feature over on #6635

@dwijnand

This comment has been minimized.

Member

dwijnand commented Jun 4, 2018

I assume there's a rebase option to fix this?

I think it's possible to fix this with --committer-date-is-author-date.

som-snytt added some commits Jun 4, 2018

Enable postfix for sbt compat file
If compiling exactly one file with a path ending
in `xsbt/Compat.scala`, enable postfix ops.

This avoids an error when sbt auto-compiles that
file, after postfixOps feature is required.

This commit does not attempt a general mechanism
for adding hacks or hooks for special cases.
@som-snytt

This comment was marked as resolved.

Contributor

som-snytt commented Jun 4, 2018

/rebuild

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jun 5, 2018

Ah right! Hrm, we'll have to release a new starr then, before we flip the required bit. Other than that, LGTM -- thanks for finessing the commits.

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 6, 2018

at https://contributors.scala-lang.org/t/lets-drop-postfix-operators/1457/11?u=sethtisue @LPTK expressed skepticism about the desirability of actually removing this in 2.14.

I think the other changes in this PR take care of disarming the footgun. so maybe the actual removal under -Xsource:2.14 isn't really necessary? I don't have a strong feeling about it, but I think it's worth a last thought before merge.

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 6, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 6, 2018

one more thought: I think the error message should be more explicit that this "feature" is actually highly undesirable, likely to be removed in a future Scala, and is still supported at all only for backward compatibility and for no other reason.

this isn't a "proceed, but with caution" type thing anymore like higherKinds and so on... this one is a "no, really, please don't"

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 20, 2018

@SethTisue Please take a look at the doc change in the last commit.

Juicier language for language feature object
Use stronger language to dissuade from postfix,
and make the descriptions more uniform.

Distinguish required and convenience flags in docs.
@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 20, 2018

@adriaanm Moved the post-starr commit to the linked PR, in case that's the right thing to do.

@SethTisue SethTisue changed the title from Make postfixOps more opt-in to Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled Jun 25, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 25, 2018

Please take a look at the doc change in the last commit

LGTM except that I'd suggest appending " (not recommended)" here

+ *   - [[postfixOps          `postfixOps`]]          enables postfix operator notation
@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jun 25, 2018

OK, I'll add " (not Seth-approved)" or " (not recommended by Seth, who knows)", whichever you prefer. I guess I'll know your preference by your reply to this comment. Actually, following Lacan, it should be, " (not recommended by Seth, the subject supposed to know)".

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 25, 2018

the -Y is an other

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jul 16, 2018

@adriaanm @SethTisue if we can move this along, I'll have time to help with community build issues.

I feel no special animus toward postfix, but hoping for clarity.

It needs restarr for the other PR to put the nail in the coffin.

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jul 17, 2018

does this PR actually make it an error? I'm not seeing that it does:

> scala
[info] Running scala.tools.nsc.MainGenericRunner -usejavacp
Welcome to Scala 2.13.0-20180716-190107-b9e039c (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_172).
Type in expressions for evaluation. Or try :help.

scala> List(1) max
               ^
       warning: postfix operator max should 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 should be explicitly enabled.
res0: Int = 1
@SethTisue

This comment has been minimized.

Member

SethTisue commented Jul 17, 2018

oh, is that because "we'll have to release a new starr"? if so, then 1) can you update the PR title, and 2) yes, let's merge this.

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented Jul 17, 2018

Yes, #6831

@som-snytt som-snytt changed the title from Make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled to Prepare to make postfixOps syntax an error (not just a warning) unless the feature is explicitly enabled Jul 17, 2018

it was done

@SethTisue SethTisue merged commit 2de8b62 into scala:2.13.x Jul 17, 2018

4 checks passed

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

This comment has been minimized.

Member

SethTisue commented Jul 17, 2018

postfix ops are looking really nervous now

@som-snytt som-snytt deleted the som-snytt:issue/post-postfix branch Jul 18, 2018

@hepin1989

This comment has been minimized.

Contributor

hepin1989 commented Nov 22, 2018

Cool~

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