Skip to content

Commit

Permalink
Scala 2.13 Scalafix: ExplicitResultTypes for implicit definitions
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
rtyley committed Oct 22, 2024
1 parent b3dabeb commit 1aea118
Show file tree
Hide file tree
Showing 21 changed files with 31 additions and 23 deletions.
Empty file added .scalafix.conf
Empty file.
2 changes: 1 addition & 1 deletion app/model/CampaignInformation.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ case class CampaignInformation(campaignType: String) {

object CampaignInformation {

implicit val trackingFormat = Jsonx.formatCaseClass[CampaignInformation]
implicit val trackingFormat: OFormat[CampaignInformation] = Jsonx.formatCaseClass[CampaignInformation]

def apply(thriftCampaignInformation: ThriftCampaignInformation): CampaignInformation =
CampaignInformation(
Expand Down
3 changes: 2 additions & 1 deletion app/model/ClientConfig.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package model

import ai.x.play.json.Encoders.encoder
import ai.x.play.json.Jsonx
import play.api.libs.json.OFormat

case class ClientConfig(username: String,
capiUrl: String,
Expand All @@ -15,5 +16,5 @@ case class ClientConfig(username: String,
)

object ClientConfig {
implicit val clientConfigFormat = Jsonx.formatCaseClass[ClientConfig]
implicit val clientConfigFormat: OFormat[ClientConfig] = Jsonx.formatCaseClass[ClientConfig]
}
2 changes: 1 addition & 1 deletion app/model/ContributorInformation.scala
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ case class ContributorInformation(

object ContributorInformation {

implicit val contributorInformationFormat = Jsonx.formatCaseClass[ContributorInformation]
implicit val contributorInformationFormat: OFormat[ContributorInformation] = Jsonx.formatCaseClass[ContributorInformation]

def apply(thriftContributorInformation: ThriftContributorInformation): ContributorInformation =
ContributorInformation(
Expand Down
3 changes: 2 additions & 1 deletion app/model/EditionalisedPage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package model
import ai.x.play.json.Jsonx
import ai.x.play.json.Encoders.encoder
import com.gu.tagmanagement.{EditionalisedPage => ThriftEditionalisedPage}
import play.api.libs.json.OFormat

case class EditionalisedPage(path: String, pageId: Long) {
def asThift = ThriftEditionalisedPage(path, pageId)
}

object EditionalisedPage {

implicit val editionalisedPageFormat = Jsonx.formatCaseClass[EditionalisedPage]
implicit val editionalisedPageFormat: OFormat[EditionalisedPage] = Jsonx.formatCaseClass[EditionalisedPage]

def apply(thriftEditionalisedPage: ThriftEditionalisedPage): EditionalisedPage =
EditionalisedPage(thriftEditionalisedPage.path, thriftEditionalisedPage.pageId)
Expand Down
3 changes: 2 additions & 1 deletion app/model/ExternalReferenceType.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import play.api.libs.json.{Format, JsPath, JsValue, Json}
import com.gu.tagmanagement.{Section => ThriftSection}

import scala.util.control.NonFatal
import play.api.libs.json.OFormat

case class ExternalReferenceType (
typeName: String,
Expand All @@ -20,7 +21,7 @@ case class ExternalReferenceType (

object ExternalReferenceType extends Logging {

implicit val sectionFormat = Jsonx.formatCaseClass[ExternalReferenceType]
implicit val sectionFormat: OFormat[ExternalReferenceType] = Jsonx.formatCaseClass[ExternalReferenceType]

def fromItem(item: Item) = try{
Json.parse(item.toJSON).as[ExternalReferenceType]
Expand Down
4 changes: 2 additions & 2 deletions app/model/HyperMedia.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import services.Config.conf


case class LinkEntity(rel: String, href: String)
object LinkEntity { implicit val jsonWrites = Json.writes[LinkEntity] }
object LinkEntity { implicit val jsonWrites: OWrites[LinkEntity] = Json.writes[LinkEntity] }

case class EmbeddedEntity[T](uri: String,
data: Option[T] = None,
Expand Down Expand Up @@ -59,7 +59,7 @@ object CollectionResponse {
case class EmptyResponse(links: Option[List[LinkEntity]] = None) {
def addLink(rel: String, href: String) = copy(links = Some(LinkEntity(rel, href) :: (links getOrElse Nil)))
}
object EmptyResponse { implicit val jsonWrites = Json.writes[EmptyResponse] }
object EmptyResponse { implicit val jsonWrites: OWrites[EmptyResponse] = Json.writes[EmptyResponse] }

object HyperMediaHelpers {
def tagUri(id: Long): String = s"https://tagmanager.${conf.pandaDomain}/hyper/tags/${id}"
Expand Down
3 changes: 2 additions & 1 deletion app/model/ImageAsset.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package model
import ai.x.play.json.Jsonx
import ai.x.play.json.Encoders.encoder
import com.gu.tagmanagement.{ImageAsset => ThriftImageAsset}
import play.api.libs.json.OFormat

case class ImageAsset(imageUrl: String, width: Long, height: Long, mimeType: String) {
def asThrift = ThriftImageAsset(imageUrl, width, height, mimeType)
Expand All @@ -15,7 +16,7 @@ case class ImageAsset(imageUrl: String, width: Long, height: Long, mimeType: Str
}

object ImageAsset {
implicit val imageAssetFormat = Jsonx.formatCaseClass[ImageAsset]
implicit val imageAssetFormat: OFormat[ImageAsset] = Jsonx.formatCaseClass[ImageAsset]

def apply(thriftImageAsset: ThriftImageAsset): ImageAsset = ImageAsset(
imageUrl = thriftImageAsset.imageUrl,
Expand Down
3 changes: 2 additions & 1 deletion app/model/PaidContentInformation.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package model
import ai.x.play.json.Jsonx
import ai.x.play.json.Encoders.encoder
import com.gu.tagmanagement.{PaidContentInformation => ThriftPaidContentInformation}
import play.api.libs.json.OFormat

case class PaidContentInformation(paidContentType: String, campaignColour: Option[String] = None) {

Expand All @@ -19,7 +20,7 @@ case class PaidContentInformation(paidContentType: String, campaignColour: Optio

object PaidContentInformation {

implicit val paidContentInformationFormat = Jsonx.formatCaseClass[PaidContentInformation]
implicit val paidContentInformationFormat: OFormat[PaidContentInformation] = Jsonx.formatCaseClass[PaidContentInformation]

def apply(thriftPaidContentInformation: ThriftPaidContentInformation): PaidContentInformation =
PaidContentInformation(
Expand Down
2 changes: 1 addition & 1 deletion app/model/PublicationInformation.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ case class PublicationInformation(

object PublicationInformation {

implicit val publicationInformationFormat = Jsonx.formatCaseClassUseDefaults[PublicationInformation]
implicit val publicationInformationFormat: OFormat[PublicationInformation] = Jsonx.formatCaseClassUseDefaults[PublicationInformation]

def apply(thriftPublicationInformation: ThriftPublicationInformation): PublicationInformation =
PublicationInformation(
Expand Down
2 changes: 1 addition & 1 deletion app/model/Reference.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ case class Reference(`type`: String, value: String, capiType: Option[String]) {

object Reference {

implicit val referenceFormat = Jsonx.formatCaseClass[Reference]
implicit val referenceFormat: OFormat[Reference] = Jsonx.formatCaseClass[Reference]

def apply(thriftReference: ThriftReference): Reference = Reference(thriftReference.`type`, thriftReference.value, thriftReference.capiType)

Expand Down
2 changes: 1 addition & 1 deletion app/model/Sponsorship.scala
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ case class DenormalisedSponsorship (

object DenormalisedSponsorship {

implicit val denormalisedSponsorshipFormat = Jsonx.formatCaseClass[DenormalisedSponsorship]
implicit val denormalisedSponsorshipFormat: OFormat[DenormalisedSponsorship] = Jsonx.formatCaseClass[DenormalisedSponsorship]

def apply(s: Sponsorship): DenormalisedSponsorship = {
new DenormalisedSponsorship(
Expand Down
2 changes: 1 addition & 1 deletion app/model/Tag.scala
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ case class DenormalisedTag (

object DenormalisedTag{

implicit val tagFormat = Jsonx.formatCaseClassUseDefaults[DenormalisedTag]
implicit val tagFormat: OFormat[DenormalisedTag] = Jsonx.formatCaseClassUseDefaults[DenormalisedTag]

def apply(t: Tag): DenormalisedTag = DenormalisedTag(
id = t.id,
Expand Down
3 changes: 2 additions & 1 deletion app/model/TagSearchResult.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package model

import ai.x.play.json.Jsonx
import ai.x.play.json.Encoders.encoder
import play.api.libs.json.OFormat

case class TagSearchResult(tags: List[Tag], count: Int)

object TagSearchResult {
implicit val tagSearchResultFormat = Jsonx.formatCaseClassUseDefaults[TagSearchResult]
implicit val tagSearchResultFormat: OFormat[TagSearchResult] = Jsonx.formatCaseClassUseDefaults[TagSearchResult]
}
2 changes: 1 addition & 1 deletion app/model/TrackingInformation.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ case class TrackingInformation(trackingType: String) {

object TrackingInformation {

implicit val trackingFormat = Jsonx.formatCaseClass[TrackingInformation]
implicit val trackingFormat: OFormat[TrackingInformation] = Jsonx.formatCaseClass[TrackingInformation]

def apply(thriftTrackingInformation: ThriftTrackingInformation): TrackingInformation =
TrackingInformation(
Expand Down
3 changes: 2 additions & 1 deletion app/model/command/BatchTagCommand.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ai.x.play.json.Encoders.encoder
import repositories._

import scala.concurrent.{Future, ExecutionContext}
import play.api.libs.json.OFormat

case class BatchTagCommand(contentIds: List[String], toAddToTop: Option[Long], toAddToBottom: List[Long], toRemove: List[Long]) extends Command {
type T = Unit
Expand Down Expand Up @@ -37,5 +38,5 @@ case class BatchTagCommand(contentIds: List[String], toAddToTop: Option[Long], t
}

object BatchTagCommand {
implicit val batchTagCommandFormat = Jsonx.formatCaseClassUseDefaults[BatchTagCommand]
implicit val batchTagCommandFormat: OFormat[BatchTagCommand] = Jsonx.formatCaseClassUseDefaults[BatchTagCommand]
}
5 changes: 3 additions & 2 deletions app/model/command/CreateTagCommand.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import play.api.Logging
import services.KinesisStreams

import scala.concurrent.{Future, ExecutionContext}
import play.api.libs.json.OFormat

case class InlinePaidContentSponsorshipCommand(
validFrom: Option[DateTime],
Expand Down Expand Up @@ -53,7 +54,7 @@ case class InlinePaidContentSponsorshipCommand(
}

object InlinePaidContentSponsorshipCommand {
implicit val inlinePaidContentSponsorshipFormat = Jsonx.formatCaseClassUseDefaults[InlinePaidContentSponsorshipCommand]
implicit val inlinePaidContentSponsorshipFormat: OFormat[InlinePaidContentSponsorshipCommand] = Jsonx.formatCaseClassUseDefaults[InlinePaidContentSponsorshipCommand]
}

case class CreateTagCommand(
Expand Down Expand Up @@ -207,5 +208,5 @@ case class CreateTagCommand(

object CreateTagCommand {

implicit val createTagCommandFormat = Jsonx.formatCaseClassUseDefaults[CreateTagCommand]
implicit val createTagCommandFormat: OFormat[CreateTagCommand] = Jsonx.formatCaseClassUseDefaults[CreateTagCommand]
}
4 changes: 2 additions & 2 deletions app/model/forms/SpreadSheet.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ object GetSpreadSheet {
}
}

implicit val filterFormat = Json.reads[SpreadSheetFilter]
implicit val format = Json.reads[GetSpreadSheet]
implicit val filterFormat: Reads[SpreadSheetFilter] = Json.reads[SpreadSheetFilter]
implicit val format: Reads[GetSpreadSheet] = Json.reads[GetSpreadSheet]
}
2 changes: 1 addition & 1 deletion app/model/image.scala
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ case class Image(imageId: String, assets: List[ImageAsset]) {
}

object Image {
implicit val imageFormat = Jsonx.formatCaseClass[Image]
implicit val imageFormat: OFormat[Image] = Jsonx.formatCaseClass[Image]

def apply(thriftImage: ThriftImage): Image = Image(
imageId = thriftImage.imageId,
Expand Down
2 changes: 1 addition & 1 deletion app/model/jobs/Step.scala
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ object Step extends Logging {
}
}

implicit val stepFormat = Format(stepReads, stepWrites)
implicit val stepFormat: Format[Step] = Format(stepReads, stepWrites)
}

// Step status is required so we know which steps require rollback
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class SponsorshipLauncher(implicit ec: ExecutionContext) extends AbstractSchedul
}

class SponsorshipExpirer(implicit ec: ExecutionContext) extends AbstractScheduledService with Logging {
implicit val username = Some("Sponsorship expirer")
implicit val username: Some[String] = Some("Sponsorship expirer")

override def runOneIteration(): Unit = try {

Expand Down

0 comments on commit 1aea118

Please sign in to comment.