Skip to content

Allow infix operators at start of line (under -Xsource:3 only)#8419

Merged
lrytz merged 2 commits intoscala:2.13.xfrom
som-snytt:topic/leading-infix
Feb 5, 2020
Merged

Allow infix operators at start of line (under -Xsource:3 only)#8419
lrytz merged 2 commits intoscala:2.13.xfrom
som-snytt:topic/leading-infix

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 14, 2019

Backport from dotty under -Xsource:2.14 -Xsource:3.

The simple expr token test was unused, and is moved
to scanner. Partests are more verbose than in dotty.

@scala-jenkins scala-jenkins added this to the 2.13.2 milestone Sep 14, 2019
@som-snytt
Copy link
Contributor Author

som-snytt commented Sep 14, 2019

Probably the reason the "simple expr token" test was not used for unary op parsing was this asymmetry:

scala> + if (true) 42 else 27
         ^
       error: illegal start of simple expression

scala> * if (true) 42 else 27
         ^
       error: ';' expected but 'if' found.

Not sure it matters which token test is applied for multiline infix. It would be kind of nice for the multiline infix case to error the same as oneline, with "illegal start of simple". "This looks like infix except for the form of the arg."

Even nicer would be to say, "Your expression is OK here, but you need a simple expression, so maybe try wrapping it in parens."

Probably every error message should say: Did you try adding parens?

@lrytz
Copy link
Member

lrytz commented Sep 24, 2019

Looks good, thanks! Leaving open for more 👀

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Sep 30, 2019
@som-snytt som-snytt force-pushed the topic/leading-infix branch from 864ac1d to 4db40e4 Compare January 23, 2020 00:38
@som-snytt
Copy link
Contributor Author

Rebased. Maybe the release notes should include a page of "dotty preview" features.

@som-snytt som-snytt force-pushed the topic/leading-infix branch from 4db40e4 to 73a2152 Compare January 24, 2020 22:28
@som-snytt
Copy link
Contributor Author

@lrytz @SethTisue Who do I gotta bribe around here? This is significant enough (and yet incremental enough) to require community build vetting.

@dwijnand
Copy link
Member

dwijnand commented Jan 31, 2020

Who do I gotta bribe around here? This is significant enough (and yet incremental enough) to require community build vetting.

I'll take a cup of coffee and an IRL chat: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3127/

See build parameters: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3127/parameters/

@som-snytt
Copy link
Contributor Author

I just hope this isn't one of those mini-features that everyone likes in theory and hates in practice, like -Xlint.

@dwijnand
Copy link
Member

(I love me some -Xlint, I just need a little -Wconf to manage it.)

@lrytz lrytz self-assigned this Feb 3, 2020
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM. I re-started the community build (https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3132), hopefully it gets a bit further this time.

Could you add a test for infix use of backticked methods? From doti

scala> class K { def x(y: Int) = 0 }
// defined class K

scala> def foo = {
     |   (new K)
     |   `x` 1
     | }
def foo: Int

scala> def foo = {
     |   (new K)
     |   x 1
     | }
3 |  x 1
  |    ^
  |    end of statement expected but integer literal found
4 |}
  |^
  |';' expected, but '}' found

@lrytz
Copy link
Member

lrytz commented Feb 3, 2020

Hmm, the community build failed during extraction of cats-effect-testing in both runs.

[cats-effect-testing] --== Extracting dependencies for cats-effect-testing ==--
[cats-effect-testing] Fetching https://github.com/djspiewak/cats-effect-testing.git
[cats-effect-testing] into /home/jenkins/workspace/scala-2.13.x-integrate-community-build/clones-0.9.16/e9bae2967a4127c42614391422d4ec3dc8624f84-cats-effect-testing
[cats-effect-testing] Took: 00h 00m 00.3s
[cats-effect-testing] Cloning /home/jenkins/workspace/scala-2.13.x-integrate-community-build/clones-0.9.16/e9bae2967a4127c42614391422d4ec3dc8624f84-cats-effect-testing
[cats-effect-testing] to /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de
[cats-effect-testing] Took: 00h 00m 00.0s
[cats-effect-testing] Fetching /home/jenkins/workspace/scala-2.13.x-integrate-community-build/clones-0.9.16/e9bae2967a4127c42614391422d4ec3dc8624f84-cats-effect-testing
[cats-effect-testing] into /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de
[cats-effect-testing] Took: 00h 00m 00.0s
[cats-effect-testing] Commit: e25ecc48c55d307f1806e2129b1fa2173672cc51
[cats-effect-testing] Extracting dependencies for: cats-effect-testing
[cats-effect-testing] Using Scala 2.13.2-bin-73a2152-SNAPSHOT during extraction.
[cats-effect-testing] Using sbt version: 1.3.7
[cats-effect-testing:error] OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=640m; support was removed in 8.0
[cats-effect-testing] [info] Updated file /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de/project/build.properties: set sbt.version to 1.3.7
[cats-effect-testing] [info] Loading settings for project root-660a6b5b5b2a59f6edd18d273dfda21a34db33de-build-build from ÿÿÿÿÿÿÿÿÿÿ~~~~dbuild~defs.sbt ...
[cats-effect-testing] [info] Loading project definition from /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de/project/project
[cats-effect-testing] [warn] There may be incompatibilities among your library dependencies; run 'evicted' to see detailed eviction warnings.
[cats-effect-testing] [info] Loading settings for project root-660a6b5b5b2a59f6edd18d273dfda21a34db33de-build from ÿÿÿÿÿÿÿÿÿÿ~~~~dbuild~defs.sbt,plugins.sbt ...
[cats-effect-testing] [info] Loading project definition from /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de/project
[cats-effect-testing] [info] Resetting onLoad... in one scope
[cats-effect-testing] [info] Loading project definition from /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de/project
[cats-effect-testing] [info] Dependencies among subprojects:
[cats-effect-testing] [info] default-sbt-project -> 
[cats-effect-testing] [info] Aggregates of subprojects:
[cats-effect-testing] [info] default-sbt-project -> 
[cats-effect-testing] [info] Building graph...
[cats-effect-testing] [info] sorting...
[cats-effect-testing] [info] These subprojects will be built: default-sbt-project
[cats-effect-testing] [warn] There may be incompatibilities among your library dependencies; run 'evicted' to see detailed eviction warnings.
[cats-effect-testing] [info] Loading settings for project root from ÿÿÿÿÿÿÿÿÿÿ~~~~dbuild~defs.sbt,build.sbt ...
[cats-effect-testing] [info] Set current project to cats-effect-testing (in build file:/home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.16/extraction/93f9252dc1509cc2d946393cbc6db14cecdac478/projects/660a6b5b5b2a59f6edd18d273dfda21a34db33de/)
[cats-effect-testing] [info] Setting extraction Scala version to 2.13.2-bin-73a2152-SNAPSHOT in 8 scopes
[cats-effect-testing] [info] Resetting onLoad... in one scope
[cats-effect-testing] [error] scala.MatchError: 2.13.2-bin-73a2152-SNAPSHOT (of class java.lang.String)
[cats-effect-testing] [error] Use 'last' for the full log.
[cats-effect-testing] Project loading failed: (r)etry, (q)uit, (l)ast, or (i)gnore? 
[cats-effect-testing:error] java.lang.RuntimeException: 
[cats-effect-testing:error] 	at scala.sys.package$.error(package.scala:27)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner.com$typesafe$dbuild$support$sbt$SbtRunner$$processCommand(SbtRunner.scala:94)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner$$anonfun$run$1$$anonfun$apply$2$$anonfun$apply$3.apply(SbtRunner.scala:75)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner$$anonfun$run$1$$anonfun$apply$2$$anonfun$apply$3.apply(SbtRunner.scala:75)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner$$anonfun$run$1.apply(SbtRunner.scala:75)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner$$anonfun$run$1.apply(SbtRunner.scala:66)
[cats-effect-testing:error] 	at sbt.IO$.withTemporaryFile(IO.scala:389)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtRunner.run(SbtRunner.scala:66)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtExtractor$.extractMetaData(DependencyExtractor.scala:107)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtBuildSystem.extractDependencies(SbtBuildSystem.scala:85)
[cats-effect-testing:error] 	at com.typesafe.dbuild.support.sbt.SbtBuildSystem.extractDependencies(SbtBuildSystem.scala:22)
[cats-effect-testing] --== End Extracting dependencies for cats-effect-testing ==--

The last green run form Feb 2 (https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3129) used the same revision of cats-effect-testing (e25ecc48c55d307f1806e2129b1fa2173672cc51), and passed. @SethTisue could you take a look?

@lrytz
Copy link
Member

lrytz commented Feb 3, 2020

@lrytz
Copy link
Member

lrytz commented Feb 3, 2020

(cc @djspiewak)

@SethTisue
Copy link
Member

we don't have a fix from Daniel for that yet (as per djspiewak/sbt-spiewak#5) — I'll hack around it today

@som-snytt
Copy link
Contributor Author

Worth repeating """\p{XDigit}""".r. Also ScalaVersion should be more available and do more.

TIL infixed backticked.

@som-snytt som-snytt force-pushed the topic/leading-infix branch from 73a2152 to 1b22680 Compare February 3, 2020 21:31
@SethTisue
Copy link
Member

cats-effects-testing thing was fixed; community build run got farther this time https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3136/

there were a bunch of failures, I suppose because -Xsource:2.14 causes some warnings to become errors instead?

Backport from dotty under `-Xsource:2.14`.

The simple expr token test was unused, and is moved
to scanner. Partests are more verbose than in dotty.
@som-snytt som-snytt force-pushed the topic/leading-infix branch from 1b22680 to 3ad1c39 Compare February 4, 2020 08:20
@lrytz
Copy link
Member

lrytz commented Feb 5, 2020

TIL infixed backticked.

Me too, by reading your code 😂

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Nice, thanks @som-snytt! Community build output didn't show anything suspicious.

@lrytz lrytz merged commit 0d111e1 into scala:2.13.x Feb 5, 2020
@lrytz
Copy link
Member

lrytz commented Feb 5, 2020

Is it too risky to enable it for 2.13?

@som-snytt som-snytt deleted the topic/leading-infix branch February 5, 2020 14:36
@som-snytt
Copy link
Contributor Author

I only do -Xsource:3.

@SethTisue SethTisue changed the title Allow infix operators at start of line Allow infix operators at start of line (under -Xsource:2.14 only) Feb 11, 2020
@SethTisue
Copy link
Member

SethTisue commented Feb 11, 2020

Is it too risky to enable it for 2.13?

I suggest we not try to get that into 2.13.2, but if we want to try for 2.13.3, a couple thoughts:

  • Perhaps we should supply an opt-out that would provide the old behavior. So that if people upgrade and something breaks and they're not sure why, they can try the opt-out to see if this is the culprit or not.
    • (And -Xsource:2.12 is would be too big a hammer, IMO.)
  • We'd definitely want to run it through the community build.

@som-snytt
Copy link
Contributor Author

It could affect a certain coder who puts a space after unary op, ! true.

➜  ~ dotr
Starting dotty REPL...
scala> def f(b: Boolean): Boolean = {
     | println("hi")
     | ! b
     | }
2 |println("hi")
3 |! b
  |^
  |value ! is not a member of Unit.
  |Note that `!` is treated as an infix operator in Scala 3.
  |If you do not want that, insert a `;` or empty line in front
  |or drop any spaces behind the operator.

Oh, I guess it would affect someone who wrote a DSL to look like markdown list:

 * start()
 * run()
 * stop()

@dwijnand
Copy link
Member

  • We'd definitely want to run it through the community build.

We did. Lukas said: "Community build output didn't show anything suspicious."

@SethTisue
Copy link
Member

SethTisue commented Feb 12, 2020

We did. Lukas said: "Community build output didn't show anything suspicious."

(I don't think that run was complete enough to give good feedback, because enabling -Xsource:2.14 enables other things too, which caused failures. But perhaps I'm being overly cautious.)

@som-snytt
Copy link
Contributor Author

We could enable under -Xsource:3:the-easy-stuff.

@SethTisue SethTisue changed the title Allow infix operators at start of line (under -Xsource:2.14 only) Allow infix operators at start of line (under -Xsource:3 only) Apr 22, 2020
dos65 added a commit to dos65/scalameta that referenced this pull request Apr 26, 2022
dos65 added a commit to dos65/scalameta that referenced this pull request Apr 26, 2022
dos65 added a commit to scalameta/scalameta that referenced this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants