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

Compilation hangs on Scala 2 using fairly long twiddles #12

Closed
francesconero opened this issue Nov 15, 2023 · 3 comments · Fixed by #13
Closed

Compilation hangs on Scala 2 using fairly long twiddles #12

francesconero opened this issue Nov 15, 2023 · 3 comments · Fixed by #13

Comments

@francesconero
Copy link
Contributor

Hello!

When I tried upgrading Skunk to 0.6.0, which uses this library, I discovered that compilation would hang, whereas with the previous version (using the old twiddle lists), it was fairly fast.

Digging in a bit, it seemed to be caused by some queries that selected a fair amount of columns (20 or so). I isolated a minimum example to demonstrate the problem: this snippet currently makes the compilation hang using Scala 2.13.12

import org.typelevel.twiddles._

1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
EmptyTuple

Scastie

I suspect the implicit conversion hlistToTuple in TwiddleCompat is causing unnecessary failed implicit searches exhibiting a quadratic complexity somewhere (even though the exact mechanism is not clear to me, it seems to be constantly trying to build a Tupler).

In fact, removing the implicit from scope makes it compile again.

import org.typelevel.twiddles.{hlistToTuple => _, _}

1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
1 *: 1 *: 1 *: 1 *:
EmptyTuple

Scastie

An approach I tried which seems to preserve the tuple conversion and not make the compiler hang is to define an implicit conversion for every tuple arity, mirroring the already existing tupleNtoHlist functions.

E.g.:

implicit def hlistToTuple2[Z <: Tuple, A, B](z: Z)(implicit
    tupler: Tupler.Aux[Z, (A, B)]
): (A, B) = tupler(z)

// and so on up to 22

This seems to help the compiler know the implicit is useless to build an HList. Is it because the output type is more explicit? I'm not sure.

I was planning on opening a PR to address this issue, as it is currently blocking our upgrade to the latest version of Skunk at work. We could keep using the legacy twiddles, but they would cause quite a lot of deprecation warnings. I just wanted to check which approach would be best to take first.

Thanks!

@mpilquist
Copy link
Member

Great find! The workaround seems reasonable to me.

@mpilquist
Copy link
Member

@francesconero I released Skunk 0.6.2 with this fix.

@francesconero
Copy link
Contributor Author

francesconero commented Nov 24, 2023 via email

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 a pull request may close this issue.

2 participants