-
Notifications
You must be signed in to change notification settings - Fork 563
Upgrade to Scala 2.13.12
#26806
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
Upgrade to Scala 2.13.12
#26806
Conversation
f17f14a
to
6f65e9f
Compare
This commit also fixes "Implicit definition should have explicit type" errors. These are warnings to ease migration to Scala 3 but we have `-Xfatal-warnings` enabled in our `scalacOptions` in `ProjectSettings.scala`.
It looks like in Scala 2.13.12 the Map.get method becomes case sensitive.
6f65e9f
to
60a3d88
Compare
@@ -31,7 +31,7 @@ case class RequestLoggerFields(request: RequestHeader, response: Option[Result], | |||
|
|||
val allowListedHeaders = (for { | |||
headerName <- allowListedHeaderNames | |||
value <- allHeadersFields.get(headerName) | |||
value <- allHeadersFields.map { case (key, value) => (key.toLowerCase, value) }.get(headerName.toLowerCase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in Scala 2.13.12 the get
method of a Map
becomes case sensitive, so I had to add this to fix this test that was failing in this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking into that because this seemed a bit strange to me, to have that code failing solely by upgrading to Scala 2.13.12. (Sorry for the long comment, I just wanted to know myself what was going on here)
TLDR: Turns out this is not a bug in Scala and also they did not really change expected behaviour (tough they did change some internals which causes your code to fail). The actual problem with this code was that you assumed that map.get here is case insensitive, which isn't defined somewhere and you can not expect that.
If you look at the code:
frontend/common/app/common/RequestLogger.scala
Lines 30 to 35 in c683b15
val allHeadersFields = request.headers.toMap.mapV(_.mkString(",")) | |
val allowListedHeaders = (for { | |
headerName <- allowListedHeaderNames | |
value <- allHeadersFields.get(headerName) | |
} yield headerName -> value).toMap |
allHeadersFields
is a Map
, not a SortedMap
or anything concrete, but a "stupid" Map
. Calling map.get
usually checks for equality to retrieve an entry. Since you do not know which concrete Map
implemention is used to back allHeadersFields
, you can not assume that retrieving elements is case sensitive, since you don't know how elements are compared internally, the Map
does not guarantee anything here.
The reason your code worked before was pure coincidence. Calling toMap
request.headers.toMap
here again just gives you a "stupid" map. Turns out, in Play this gives you a TreeMap
implemention with a CaseInsensitiveOrdered
ordering, see here:
val builder = TreeMap.newBuilder[String, Seq[String]](CaseInsensitiveOrdered)
Now, calling mapV
on this method "transforms" this map
, you can see that here:
def mapV[U](f: map.V => U): Map[map.K, U] = map(coll).toMap.transform { case (_,v) => f(v) }
The toMap
call here (which basically is toMap
on the given map) in that line is calling Scala's internal code which in Scala 2.13.11 would return just the same instance that was given already, meaning it just returns the exact same map with the internal CaseInsensitiveOrdered
ordering.
In Scala 2.13.12 however they changed some internals (because of other reasons) you get back a new instance of Map
which however does not have the same ordering anymore. So the case-insensitive ordering from Play's internal code was lost and that is why your code breaks. Actually this is totally ok, because toMap
does not make any promises which map it returns, not does it make promises on ordering.
The internal code they changed in Scala can be seen here, it's the from
method:
In Scala 2.13.11 the matching case was:
case m: Map[K, V] => m
which just returns the same instance again (which was the same TreeMap
with the ordering)
In Scala 2.13.12 however that case was removed and the code falls back to:
case _ => newBuilder[K, V].addAll(it).result()
which creates a new map, but as you can see the newBuilder
does not get an ordering
passed (turns out it then returns an Map2
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! This makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rtyley I think you will be interested in this!
@@ -32,7 +32,7 @@ object ProjectSettings { | |||
Compile / packageDoc / publishArtifact := false, | |||
Compile / doc / sources := Seq.empty, | |||
Compile / doc := target.map(_ / "none").value, | |||
scalaVersion := "2.13.10", | |||
scalaVersion := "2.13.12", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine!
Seen on ADMIN-PROD (merged by @ioannakok 12 minutes and 35 seconds ago)
|
Seen on FRONTS-PROD (merged by @ioannakok 14 minutes and 23 seconds ago)
|
What does this change?
Upgrades scala version to
2.13.12
.What is the value of this and can you measure success?
-Xfatal-warnings
flag enabled. This also helps to ease migration to Scala 3.With Scala
2.13.10
the compiler was crashing.Screenshots
Instead of crashing the compiler is producing a compilation error which we can now work on fixing in #26783
Checklist