Scala 2.13: Fix ambiguous overloaded JsonComponent method

[rtyley]

Don't worry, this PR looks big but it's only a rename of a method that's called in quite a few places!

The Frontend codebase compiles just fine under Scala 2.12 right now, but if we switch to the Scala 2.13 compiler (ie for #25190) it suddenly starts giving a lot of 'ambiguous reference to overloaded definition' errors, eg here:

frontend/sport/app/football/controllers/MoreOnMatchController.scala

Lines 396 to 397 in 5e17a2f

	JsonComponent(
	"items" -> Json.arr(

[error] ambiguous reference to overloaded definition
[error] both method apply in object JsonComponent of type (items: (String, Any)*)(implicit request: play.api.mvc.RequestHeader): model.Cached.RevalidatableResult
[error] and  method apply in object JsonComponent of type [A](value: A)(implicit request: play.api.mvc.RequestHeader, writes: play.api.libs.json.Writes[A]): model.Cached.RevalidatableResult
[error] match argument types ((String, play.api.libs.json.JsArray)) and expected result type model.Cached.CacheableResult

...the Scala compiler is saying it can't work out which of these 2 methods we're trying to call:

A) This one that accepts any type T at all, so long as an instance of play.api.libs.json.Writes[T] is available:

frontend/common/app/common/JsonComponent.scala

Lines 17 to 21 in 5a6d148

	def apply[A](value: A)(implicit request: RequestHeader, writes: Writes[A]): RevalidatableResult =
	resultFor(
	request,
	Json.stringify(Json.toJson(value)),
	)

B) This one that accepts multiple (String, Any) pairs:

frontend/common/app/common/JsonComponent.scala

Lines 36 to 39 in 5a6d148

	def apply(items: (String, Any)*)(implicit request: RequestHeader): RevalidatableResult = {
	val json = jsonFor(items: _*)
	resultFor(request, json)
	}

Why does the code only fail under Scala 2.13?

The Scala 2.12 compiler was unfortunately being too slack on ambiguity, and Scala 2.13 is actually being much more sensible! Under Scala 2.12, the compiler would arbitrarily prefer the 'B' varargs method that takes (String, Any) pairs, even though the 'A' method would have been a valid choice if only a single pair was passed in (and there wasn't any reasonable way for a programmer to guess which method it was going to choose). It shouldn't have made such an arbitrary choice - and Scala 2.13 corrects that.

You can see the arbitrary nature of Scala 2.12's choice of method here, in this online example:

When you have both the 'pairs' & 'writes' versions of the methods available- so foo() is overloaded- there is no good reason why Scala 2.12 should pick either - but it always picks the 'pairs' one.

What's the fix?

We need to remove the ambiguity, and make certain that when we call JsonComponent there's only ever exactly 1 method that matches our invocation! We can do that by renaming one of the two apply() methods.

We could rename either method, but it's actually preferable to change method A (that uses Writes[T]). That's because if we were to rename B, and then fix all the compilation errors, we wouldn't see places where the Scala 2.12 compiler was originally choosing B, but can still choose A - the compiler will just start using the the A method without raising a compilation error. Unless we realise that, we'll actually be unintentionally switching parts of our codebase from using B to A... potentially changing the results?

If we rename 'A', then the resulting compilation errors across the codebase will be ALL places that 'A' was getting called - because the Scala 2.12 compiler only called the 'A' method when it had no choice, ie when the (String, Any) form of method 'B' was not available.

[rtyley]

Two things are happening here:

1. We're moving from the ambiguous apply() method invocation (ie JsonComponent()) to the unambiguous JsonComponent.fromWritable() invocation - it's still the same method implementation we're calling, it's just the method is now explicitly named.
2. We're removing a superfluous call to Json.toJson() - that was always superfluous, it's just clearer to see now when we look at the JsonComponent.fromWritable() method that it also does a call to Json.toJson(), so there's no point in doing it twice.

[ioannakok]

🎉

[jamesgorrie]

Really well considered solution on which method to rename. Love it.

[prout-bot]

Seen on PROD (merged by @rtyley 15 minutes and 3 seconds ago)

• Check your changes on www.theguardian.com ✔️
• Keep an eye on the deploy dashboard 📉