Skip to content

Conversation

rtyley
Copy link
Member

@rtyley rtyley commented Oct 11, 2024

This PR sits on top of:

...with all those updates out of the way, this PR to upgrade to Scala 2.13 is pretty small:

Completing this upgrade is also a step in unblocking:

Testing

This has been deployed to CODE, and looks good:

image

@JamieB-gu JamieB-gu force-pushed the upgrade-to-scala-2.13 branch from 4c504f9 to c9811fa Compare October 22, 2024 15:54
rtyley added a commit that referenced this pull request Oct 22, 2024
This is a precursor to:

* #536

Scala 2.13.11 added a warning that implicit definitions [should](
https://nrinaudo.github.io/scala-best-practices/tricky_behaviours/type_implicits.html) have an explicit type (because implicit resolution is already complicated enough for the compiler, and things like file-order can actually make a difference to whether implicit scopes are correctly searched: scala/bug#8697 (comment)):

* scala/scala#10083

The warning looks like this:

```
[error] ~/code/presence-indicator/app/autoscaling/Notification.scala:7:16: Implicit definition should have explicit type (inferred play.api.libs.json.Reads[autoscaling.Notification]) [quickfixable]
[error]   implicit val jsonReads = Json.reads[Notification]
[error]                ^
```

...this prepares us for Scala 3, where the explicit type is _required_.

More widely, beyond implicit definitions, in **library** code, the best practice is to always add an explicit type to all your **public** members, even when you're happy with what's being inferred - otherwise you can unintentionally break binary-compatibility just by changing the _implementation_ of a field:

* https://docs.scala-lang.org/scala3/guides/migration/incompat-type-inference.html
* https://nrinaudo.github.io/scala-best-practices/binary_compat/explicit_type_annotations.html
* https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes.html

## Automatically fixing this code issue

Scalafix actually does a better job than `-quickfix` for this particular task, because it adds imports if it needs to, so that you end up with this in your code:

```
implicit val jsonReads Reads[Notification] = Json.reads[Notification]
```

...rather than something like:

```
implicit val jsonReads Reads[com.gu.blah.foo.bar.Notification] = Json.reads[Notification]
```

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

In this commit, we're only trying to fix the implicit definitions, so I've added this in a new `.scalafix.conf` config file:

```
ExplicitResultTypes.onlyImplicits = true
```

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 ExplicitResultTypes
```
@rtyley rtyley added Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 scalafix-rule-update Automatic rewrite using Scalafix (https://scalacenter.github.io/scalafix/) labels Oct 22, 2024
rtyley added a commit that referenced this pull request Oct 22, 2024
This is a precursor to:

* #536

Scala 2.13.11 added a warning that implicit definitions [should](
https://nrinaudo.github.io/scala-best-practices/tricky_behaviours/type_implicits.html) have an explicit type (because implicit resolution is already complicated enough for the compiler, and things like file-order can actually make a difference to whether implicit scopes are correctly searched: scala/bug#8697 (comment)):

* scala/scala#10083

The warning looks like this:

```
[error] ~/code/presence-indicator/app/autoscaling/Notification.scala:7:16: Implicit definition should have explicit type (inferred play.api.libs.json.Reads[autoscaling.Notification]) [quickfixable]
[error]   implicit val jsonReads = Json.reads[Notification]
[error]                ^
```

...this prepares us for Scala 3, where the explicit type is _required_.

More widely, beyond implicit definitions, in **library** code, the best practice is to always add an explicit type to all your **public** members, even when you're happy with what's being inferred - otherwise you can unintentionally break binary-compatibility just by changing the _implementation_ of a field:

* https://docs.scala-lang.org/scala3/guides/migration/incompat-type-inference.html
* https://nrinaudo.github.io/scala-best-practices/binary_compat/explicit_type_annotations.html
* https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes.html

## Automatically fixing this code issue

Scalafix actually does a better job than `-quickfix` for this particular task, because it adds imports if it needs to, so that you end up with this in your code:

```
implicit val jsonReads Reads[Notification] = Json.reads[Notification]
```

...rather than something like:

```
implicit val jsonReads Reads[com.gu.blah.foo.bar.Notification] = Json.reads[Notification]
```

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

In this commit, we're only trying to fix the implicit definitions, so I've added this in a new `.scalafix.conf` config file:

```
ExplicitResultTypes.onlyImplicits = true
```

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 ExplicitResultTypes
```
@rtyley rtyley force-pushed the upgrade-to-scala-2.13 branch from c9811fa to b7e6f41 Compare October 22, 2024 16:31
case("from") => sponsorships.sortBy(_.validFrom.map(_.getMillis).getOrElse(0l))
case("from") => sponsorships.sortBy(_.validFrom.map(_.getMillis).getOrElse(0L))
Copy link
Member Author

Choose a reason for hiding this comment

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

Scala 2.13 now warns on using a lower-case l for Long literals: scala/scala#7685

[warn] /Users/Roberto_Tyley/code/tagmanager/app/modules/clustersync/NodeStatusRepository.scala:40:8: [rewritten by -quickfix] Lowercase el for long is not recommended because it is easy to confuse with numeral 1; use uppercase L instead
[warn]       1l
[warn]        ^

@rtyley rtyley removed the scalafix-rule-update Automatic rewrite using Scalafix (https://scalacenter.github.io/scalafix/) label Oct 23, 2024
@rtyley rtyley marked this pull request as ready for review October 23, 2024 08:30
@rtyley rtyley merged commit e296b50 into main Oct 23, 2024
3 checks passed
@rtyley rtyley deleted the upgrade-to-scala-2.13 branch October 23, 2024 08:30
@prout-bot
Copy link

Seen on PROD (merged by @rtyley 6 minutes and 25 seconds ago) Please check your changes!

@rtyley
Copy link
Member Author

rtyley commented Oct 23, 2024

This has been deployed to PROD, and looks good! ✨

image

@emdash-ie emdash-ie mentioned this pull request Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 Seen-on-PROD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants