Skip to content
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

Cross-compile for Scala 3 #561

Merged
merged 19 commits into from
Sep 4, 2021
Merged

Cross-compile for Scala 3 #561

merged 19 commits into from
Sep 4, 2021

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Jul 29, 2021

Introduced a couple of little warts to retain identical keys for the tests (to avoid having to refactor them too much). Added comments on those locations.

Most of the changes are just formatting from bumping the scalafmt version...

Also (think I've) fixed the docs/mdoc call now 🤞 seems to work locally

Hopefully this is useful to someone else

@hughsimpson hughsimpson changed the title (WIP) Cross-compile for Scala 3 Cross-compile for Scala 3 Jul 29, 2021
@@ -50,7 +50,7 @@ trait CacheKeySpecCommon extends Suite with Matchers with BeforeAndAfter {

}

class AClass[F[_]](implicit cache: Cache[F, Int]) {
class AClass[F[_]]()(implicit cache: Cache[F, Int]) {
Copy link
Contributor Author

@hughsimpson hughsimpson Jul 29, 2021

Choose a reason for hiding this comment

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

Added the extra () here just so they're included in the scala 3 generated cache keys (otherwise CacheKeyIncludingConstructorParamsSpec."work for a method inside a class" fails as it would generate "scalacache.memoization.AClass(${cache.toString}).insideClass(1)" rather than "scalacache.memoization.AClass()(${cache.toString}).insideClass(1)")

For compatibility on this, it should be enough to check if the first element of the macro call to .paramss is implicit, and prefix an empty list in that case, but it feels rather OTT, especially assuming that most usages of this lib will be using a local, rather than a distributed, cache

@@ -33,7 +33,7 @@ object MethodCallToStringConverter {

private def appendClassNamePart(sb: JStringBuilder)(className: String): Unit = {
if (className.nonEmpty) {
sb.append(className)
sb.append(className.stripSuffix("$"))
Copy link
Contributor Author

@hughsimpson hughsimpson Jul 29, 2021

Choose a reason for hiding this comment

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

Ugly... Just to retain the exact same format for keys generated for the tests. Won't even do that in general, since an object Foo { object Bar { def baz = memoize { ... } }} would have Foo$.Bar.baz in scala 3 rather than Foo.Bar.baz - so might be worth figuring out how to rewrite the test assertions appropriately to acknowledge the slightly different className formats between scala 2 and scala 3...

Of course a String.replace("$.", ".") at the end would permit full compatibility on that front, but would be more expensive... I don't know whether full compatibility of keys between Scala versions (useful, I guess, for Redis) is a goal of this project, so I've avoided it here

@@ -42,7 +42,7 @@ object MethodCallToStringConverter {
sb: JStringBuilder
)(className: String, constructorParamss: IndexedSeq[IndexedSeq[Any]]): Unit = {
if (className.nonEmpty) {
sb.append(className)
sb.append(className.stripSuffix("$"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto (see comment on the other stripSuffix)

@@ -99,7 +99,7 @@ class Bar(a: Int) {
then you want the cache key to depend on the values of both `a` and `b`. In that case, you need to use a different implementation of [MethodCallToStringConverter](https://github.com/cb372/scalacache/blob/master/modules/core/shared/src/main/scala/scalacache/memoization/MethodCallToStringConverter.scala), like this:

```scala mdoc:silent
implicit val cacheConfig = CacheConfig(
implicit lazy val cacheConfig: CacheConfig = CacheConfig(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the implicit val catsCache: Cache[IO, Cat] = MemcachedCache("localhost:11211") earlier on in this file constructs a forward reference to this implicit val (presumably it didn't before the tut->mdoc change); so this needs to be lazy or there's a weird NPE/AbstractMethodError (depending on scala version) thrown when running docs/mdoc

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the rest works without this snippet, so I'd rather remove the lazy but exclude this snippet from mdoc.

@kubukoz kubukoz mentioned this pull request Aug 9, 2021
val sym: Symbol = Symbol.spliceOwner

def hasCacheKeyExcludeAnnotation(s: Symbol): Boolean = s.annotations.exists {
// case Apply(Select(New(Ident("cacheKeyExclude")), _), _) => true // Don't know why this doesn't match...
Copy link
Collaborator

Choose a reason for hiding this comment

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

funny, summon[TypeTest[Tree, Ident]].unapply(o) gives me a None too. Guess that's what's used, since Ident is a type member, thus prone to type erasure, so TypeTest will be used underneath (like ClassTag would be used in scala 2).

No idea why it doesn't match though. Maybe it's worth reporting somewhere...

@@ -99,7 +99,7 @@ class Bar(a: Int) {
then you want the cache key to depend on the values of both `a` and `b`. In that case, you need to use a different implementation of [MethodCallToStringConverter](https://github.com/cb372/scalacache/blob/master/modules/core/shared/src/main/scala/scalacache/memoization/MethodCallToStringConverter.scala), like this:

```scala mdoc:silent
implicit val cacheConfig = CacheConfig(
implicit lazy val cacheConfig: CacheConfig = CacheConfig(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the rest works without this snippet, so I'd rather remove the lazy but exclude this snippet from mdoc.

@kubukoz
Copy link
Collaborator

kubukoz commented Sep 4, 2021

This is good, thanks :)
updated with master and going to merge on green.

@kubukoz kubukoz merged commit 7d93888 into cb372:master Sep 4, 2021
@hughsimpson hughsimpson deleted the scala_3 branch September 14, 2021 17:49
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

Successfully merging this pull request may close these issues.

2 participants