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

Avoid repeatedly evaluating the source if it's a complex expression #115

Merged
merged 3 commits into from
Sep 11, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import scala.reflect.ClassTag

package object quicklens {

extension [S, A](inline obj: S)
// #114: obj shouldn't be inline since we want to reference the parameter by-name, rather then embedding the whole
// expression whenever obj is used; this is especially important for chained .modify invocations
extension [S, A](obj: S)
/** Create an object allowing modifying the given (deeply nested) field accessible in a `case class` hierarchy via
* `path` on the given `obj`.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.softwaremill.quicklens.test

import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers

class CompileTimeTest extends AnyFlatSpec with Matchers {
// #114
it should "not compile for too long in case of chained modify invocations" in {
val start = System.currentTimeMillis()
assertDoesNotCompile("""
Copy link
Contributor

Choose a reason for hiding this comment

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

While the code currently does not compile, would it really be a bug if it did? It does compile in Scala 2. I think assertDoesNotCompile should not be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's in Scala3 sources as assertDoesNotCompile is a Scala3-only thing as far as I know (in ScalaTest).

As for this not compiling... I though that was the point showing the compiler timing issue (which is fixed here), but now I see there are two problems

Copy link
Member Author

Choose a reason for hiding this comment

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

I've extracted this as a separate issue, #116

Copy link
Member Author

Choose a reason for hiding this comment

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

And changed the type here to an unrelated one, which definitely shouldn't work :)

case class B(a1: Double, a2: Double, a3: Double, a4: Double, a5: Double)
case class C(b: B)

import com.softwaremill.quicklens.*

val c = C(B(1, 1, 1, 1, 1))
c
.modify(_.b.a1).setTo(0)
.modify(_.b.a2).setTo(0)
.modify(_.b.a3).setTo(0)
.modify(_.b.a4).setTo(0)
.modify(_.b.a5).setTo(0)
""")
val end = System.currentTimeMillis()
(end - start) shouldBe <=(5000L) // that's a lot anyway
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package com.softwaremill.quicklens

import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers

class RepeatedModifyTest extends AnyFlatSpec with Matchers {
import RepeatedModifyTest._

it should "properly handle repeated modify invocations for different fields" in {
val c = C(B(1, 1, 1, 1, 1))
c
.modify(_.b.a1)
.setTo(0.0d)
.modify(_.b.a2)
.setTo(0.0d)
.modify(_.b.a3)
.setTo(0.0d)
.modify(_.b.a4)
.setTo(0.0d)
.modify(_.b.a5)
.setTo(0.0d) shouldBe C(B(0, 0, 0, 0, 0))
}

it should "properly handle repeated modify invocations for the same field" in {
val c = C(B(1, 1, 1, 1, 1))
c
.modify(_.b.a1)
.setTo(0.0d)
.modify(_.b.a1)
.setTo(1.0d)
.modify(_.b.a1)
.setTo(2.0d)
.modify(_.b.a1)
.setTo(3.0d)
.modify(_.b.a1)
.setTo(4.0d) shouldBe C(B(4, 1, 1, 1, 1))
}
}

object RepeatedModifyTest {
case class B(a1: Double, a2: Double, a3: Double, a4: Double, a5: Double)
case class C(b: B)
}