Skip to content

ExplicitNonNullaryApply: insert () for non-nullary method application #204

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

Closed
olafurpg opened this issue Jun 8, 2017 · 8 comments
Closed

Comments

@olafurpg
Copy link
Contributor

olafurpg commented Jun 8, 2017

Dotty will require explicit parentheses () for non-nullary method applications, see scala/scala3#2571

It would be nice to have a scalafix rewrite ExplicitNullaryApply that produces diffs like this

- List.newBuilder[Int].result
+ List.newBuilder[Int].result()

For a larger example, see scala/scala3#2716

Currently blocked by scalameta/scalameta#930

@olafurpg
Copy link
Contributor Author

Implemented the rewrite here https://github.com/scalacenter/scalafix/compare/master...olafurpg:no-auto-apply?expand=1 Just waiting for release of scalameta/scalameta#1043 to get Denotation.isJavaDefined

@olafurpg
Copy link
Contributor Author

I continued working on this issues but got stuck on an issue with Term.ApplyType

https://github.com/scalacenter/scalafix/compare/master...olafurpg:no-auto-apply?expand=1#diff-4b9700111672b3f4b8593bb6dcbbf614R36

We're very close to solving this, but I feel the denotation API is still a bit too raw for these rewrites.

@xeno-by
Copy link
Contributor

xeno-by commented Aug 10, 2017

@olafurpg What would you like to see in the denotation API?

@olafurpg
Copy link
Contributor Author

For methods I'd love to get some sort of Defn.Def structure. Currently I do Denotation.info.contains("()") to check if a method is nullary, I first did .startsWith("())but that fails withT`. Implementing rewrites against the strings is too fragile IMO.

@olafurpg olafurpg modified the milestone: v0.5 Aug 16, 2017
@olafurpg
Copy link
Contributor Author

olafurpg commented Aug 23, 2017

Made a lot of progress on this rewrite today. The logic is sound and I've tested it on scala-library with nice success. There is one major blocker however, the rewrite is triggered for nullary scala 2.x defined methods that override java methods. The scala compiler doesn't store this information in its signatures, so it's not possible to work around this except to fix it in scala/scala. I think the fix in scala/scala would require changes to the scala signatures so we likely can't ship this rewrite except for scala 2.13, if we manage to get the change into the compiler.

It seems dotty still permits non-nullary apply on nullary scala 2.x methods that override java methods scala/scala3#3012

olafurpg added a commit to olafurpg/scala that referenced this issue Aug 23, 2017
@olafurpg olafurpg removed this from the v0.5 milestone Aug 24, 2017
@olafurpg olafurpg changed the title ExplicitNullaryApply: insert () for nullary method application ExplicitNullaryApply: insert () for non-nullary method application Aug 28, 2017
@olafurpg olafurpg changed the title ExplicitNullaryApply: insert () for non-nullary method application ExplicitNonNullaryApply: insert () for non-nullary method application Aug 28, 2017
@olafurpg
Copy link
Contributor Author

olafurpg commented Feb 28, 2018

A possible workaround for this blocker issue would be to skip methods that override non-nullary methods. This would avoid false-positives at the cost of introducing a few potential false-negatives.

@olafurpg
Copy link
Contributor Author

However can you rewrite the call site when the receiver type is a structural type (as shown in the example above)?

From scala/scala3#4026 (comment)

@olafurpg olafurpg added the dotty label Aug 27, 2018
@olafurpg
Copy link
Contributor Author

Migration rules for Dotty will live in a different directory than the scalacenter/scalafix so that they can evolve at a different pace than Scalafix APIs. Thanks to recent improvements, custom rules can still have the identical UX as built-in rules.

  • automatically installed by sbt-scalafix
  • tab completions in the sbt shell like built-in rules
  • documentation on the main Scalafix website

rtyley added a commit to guardian/tagmanager that referenced this issue Oct 22, 2024
…`()` is deprecated

Scala 2.13 deprecates (with PR scala/scala#8833) the old behaviour of Scala that zero-parameter methods could be called with either one or zero pairs of parenthesis - ie if you have a method `def foo()` you could call it as `foo()` or just `foo`. With Scala 3, you have to match the number of brackets *used in the method declaration* when you call it - so you'd _have_ to use `foo()` or you'd get an error like this:

```
[error] ~/code/presence-indicator/app/actor/OpenSocketActor.scala:79:7: Auto-application to `()` is deprecated. Supply the empty argument list `()` explicitly to invoke method sender,
[error] or remove the empty argument list from its definition (Java-defined methods are exempt).
[error] In Scala 3, an unapplied method like this will be eta-expanded into a function. [quickfixable]
[error]       sender ! Map(serverId -> (LiveActors(connectionPing, subscription)))
[error]       ^
```

Java methods are exempt from this restriction - you can call either `hashCode()` (which, in Java, is how the method _has_ to be defined, with empty brackets) or just `hashCode` (which is how that method would have been declared if it was declared in Scala, in Scala methods with no side-effects should be declared without brackets: https://docs.scala-lang.org/style/method-invocation.html#arity-0).

## Automatically fixing this code issue

There are two possible ways of automating this code fix - in this small project, they both produce the same code changes:

### Fixing if the project is already on Scala 2.13 - use `-quickfix` in `scalac`

You can use the `-quickfix` support added to Scala 2.13.12 with scala/scala#10482:

Add either of these to the `scalacOptions` in `build.sbt`:

* `"-quickfix:any"`  ...to apply *all* available quick fixes
* `"-quickfix:msg=Auto-application"`  ...to apply quick fixes where the message contains "Auto-application"

Then run `compile` on the sbt console - the first compile will still fail, but it will subtly change the error message to say `[rewritten by -quickfix]` - your files have been edited to receive the fix:

```
[error] /Users/Roberto_Tyley/code/presence-indicator/app/actor/OpenSocketActor.scala:79:7: [rewritten by -quickfix] Auto-application to `()` is deprecated. Supply the empty argument list `()` explicitly to invoke method sender,
[error] or remove the empty argument list from its definition (Java-defined methods are exempt).
[error] In Scala 3, an unapplied method like this will be eta-expanded into a function.
[error]       sender ! Map(serverId -> (LiveActors(connectionPing, subscription)))
[error]       ^
```

...run `compile` a second time, and compiler will be much happier.

Examples of other PRs using `-quickfix` to fix this code issue:

* guardian/ophan#5719

### Fixing while still on Scala 2.12 - use Scalafix

Fixing this everywhere in a project can be tedious, but thankfully there is a `ExplicitNonNullaryApply` Scalafix rule to fix this in the https://github.com/lightbend-labs/scala-rewrites project.

The Scalafix rule needs to be run while the project is still on Scala 2.12, not Scala 2.13 (otherwise sbt will say: "Error downloading ch.epfl.scala:sbt-scalafix;sbtVersion=1.0;scalaVersion=2.13:0.13.0").

Once the Scalafix plugin is made available to sbt (by adding `addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.13.0")`
to either `project/plugins.sbt` or `~/.sbt/1.0/plugins.sbt`), you can run these commands on the sbt prompt to automatically generate the changes in this PR:

```
scalafixEnable
scalafixAll dependency:[email protected]:scala-rewrites:0.1.5
```

Examples of other PRs using Scalafix to fix this code issue:

* guardian/mobile-apps-api#2728
* guardian/presence-indicator#196

See also:

* scalacenter/scalafix#204
* lightbend-labs/scala-rewrites#14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants