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

SI-7470 implements fundep materialization #2499

Closed
wants to merge 1 commit into
base: 2.10.x
from

Conversation

Projects
None yet
2 participants
@xeno-by
Member

xeno-by commented May 7, 2013

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by May 7, 2013

Member

This pull request builds upon #2494, enabling implicit macros to affect type inference. Unfortunately we had a disagreement about whether we want to include this functionality in trunk, so I moved the controversial bits into a separate pull request. The difference between #2494 and this pull request is a single commit: scalamacros/kepler@ticket/5923...topic/iso.

Member

xeno-by commented May 7, 2013

This pull request builds upon #2494, enabling implicit macros to affect type inference. Unfortunately we had a disagreement about whether we want to include this functionality in trunk, so I moved the controversial bits into a separate pull request. The difference between #2494 and this pull request is a single commit: scalamacros/kepler@ticket/5923...topic/iso.

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by May 7, 2013

Member

The motivation

trait Iso[T, U] {
  def to(t : T) : U
  def from(u : U) : T
}
implicit def materializeIso[T, U]: Iso[T, U] = macro ???

We have a multi-parametric Iso[T, U] type class, which represents isomorphisms between case classes (T) and HLists (U). Okay, so we have a fundep T -> U, and now we want to materialize that fundep.

The problem

case class Foo(i: Int, s: String, b: Boolean)
def foo[C, L](c: C)(implicit iso: Iso[C, L]): L = iso.to(c)
foo(Foo(23, "foo", true))

We cannot use the regular materialization technique, because there's a problem with type inference. As shown on the snippet above, when inferring an implicit argument for an invocation of foo, we look for an implicit of type Iso[C, ?L]. However macros cannot expand when a macro application contains undetparams, so before expanding the implicit macro, the macro engine will instantiate undetparams, turning ?L into Nothing. And then even if materializeIso expands into new Iso[C, Int :: String :: Boolean :: HNil] { ... }, it wouldn't typecheck against Iso[C, Nothing], which makes materialization of fundep multiparametric type classes impossible.

The solution

The patch consists of two simple ideas, both having just a handful of locs in their implementation:

  1. Even if a macro application contains undetparams, let it expand.
  2. When typechecking the result of macro expansion, replace all undetparams in the expected type with wildcards.

In the Iso[C, ?L] case, the materialization macro doesn't care about ?L being not inferred, because it knows exactly what to infer it to. So it will simply expand into new Iso[C, Int :: String :: Boolean :: HNil] { ... } and be done with the it. When typechecking the expansion, macro engine typechecks it against Iso[C, ?] instead of Iso[C, ?L], which works out. And now type inference sees that the type of the implicit candidate is Iso[C, Int :: String :: Boolean :: HNil], so it figures out the correct value for ?L.

tl;dr

Even if you can't infer all the type parameters of implicit being looked up, let the macro expand. All the undetparams will later be inferred from the result of expansion. The implementation of that technique is extremely simple - just 8 lines of code.

Member

xeno-by commented May 7, 2013

The motivation

trait Iso[T, U] {
  def to(t : T) : U
  def from(u : U) : T
}
implicit def materializeIso[T, U]: Iso[T, U] = macro ???

We have a multi-parametric Iso[T, U] type class, which represents isomorphisms between case classes (T) and HLists (U). Okay, so we have a fundep T -> U, and now we want to materialize that fundep.

The problem

case class Foo(i: Int, s: String, b: Boolean)
def foo[C, L](c: C)(implicit iso: Iso[C, L]): L = iso.to(c)
foo(Foo(23, "foo", true))

We cannot use the regular materialization technique, because there's a problem with type inference. As shown on the snippet above, when inferring an implicit argument for an invocation of foo, we look for an implicit of type Iso[C, ?L]. However macros cannot expand when a macro application contains undetparams, so before expanding the implicit macro, the macro engine will instantiate undetparams, turning ?L into Nothing. And then even if materializeIso expands into new Iso[C, Int :: String :: Boolean :: HNil] { ... }, it wouldn't typecheck against Iso[C, Nothing], which makes materialization of fundep multiparametric type classes impossible.

The solution

The patch consists of two simple ideas, both having just a handful of locs in their implementation:

  1. Even if a macro application contains undetparams, let it expand.
  2. When typechecking the result of macro expansion, replace all undetparams in the expected type with wildcards.

In the Iso[C, ?L] case, the materialization macro doesn't care about ?L being not inferred, because it knows exactly what to infer it to. So it will simply expand into new Iso[C, Int :: String :: Boolean :: HNil] { ... } and be done with the it. When typechecking the expansion, macro engine typechecks it against Iso[C, ?] instead of Iso[C, ?L], which works out. And now type inference sees that the type of the implicit candidate is Iso[C, Int :: String :: Boolean :: HNil], so it figures out the correct value for ?L.

tl;dr

Even if you can't infer all the type parameters of implicit being looked up, let the macro expand. All the undetparams will later be inferred from the result of expansion. The implementation of that technique is extremely simple - just 8 lines of code.

[nomaster] SI-7470 implements fundep materialization
This fix provides implicit macros with an ability to affect type inference
in a more or less sane manner. That's crucial for materialization of
multi-parametric type class instances (e.g. Iso's from shapeless).
Details of the technique can be found in comments.

NOTE: The changes introduced here are good from the conceptual point of view,
but are sub-standard as far as clarity and cleanliness are concerned.
As the latest advances in implicit macrology show, there's no need for
an elaborate mechanism of keeping track of undetparams in Macros.scala,
therefore we should really clean that stuff out. However, this is 2.10.x,
and I want to minimize the surface of code changes in order to make sure
I don't break anything.

Having said all that, I'd like to note that this commit is only targeted
at 2.10.x. There's no reason for it to be merged into master.
@@ -706,6 +706,11 @@ trait Macros extends scala.tools.reflect.FastTrack with Traces {
var expectedTpe = expandee.tpe
if (isNullaryInvocation(expandee)) expectedTpe = expectedTpe.finalResultType
var undetparams = List[Symbol]()

This comment has been minimized.

@adriaanm

adriaanm May 14, 2013

Member

This should probably use approximateAbstracts. undetparams is not used except for replacing them with wildcards, so it should be fine.

@adriaanm

adriaanm May 14, 2013

Member

This should probably use approximateAbstracts. undetparams is not used except for replacing them with wildcards, so it should be fine.

This comment has been minimized.

@xeno-by

xeno-by Aug 12, 2013

Member

This comment has been minimized.

@xeno-by

xeno-by Aug 12, 2013

Member

I also tried to change approximateAbstracts to work for this case and see what breaks. You can follow the experiment live at https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/987/console.

@xeno-by

xeno-by Aug 12, 2013

Member

I also tried to change approximateAbstracts to work for this case and see what breaks. You can follow the experiment live at https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/987/console.

var undetparams = List[Symbol]()
def traverse(sym: Symbol) = if (sym.isTypeParameter && !sym.isSkolem) undetparams ::= sym
expectedTpe.foreach(sub => traverse(sub.typeSymbol))
expectedTpe = deriveTypeWithWildcards(undetparams)(expectedTpe)

This comment has been minimized.

@adriaanm

adriaanm May 14, 2013

Member

Can this be expressed with expectedTpe a val, not a var?

Let's call the approximated expected type ptApprox

@adriaanm

adriaanm May 14, 2013

Member

Can this be expressed with expectedTpe a val, not a var?

Let's call the approximated expected type ptApprox

if (shouldInstantiate) {
forced += delayed
typer.infer.inferExprInstance(delayed, typer.context.extractUndetparams(), pt, keepNothings = false)
macroExpand(typer, delayed, mode, WildcardType)

This comment has been minimized.

@adriaanm

adriaanm May 14, 2013

Member

Shouldn't this use at least ptApprox as expected type? (Or pt with undetparams replaced by their inferred types.)

@adriaanm

adriaanm May 14, 2013

Member

Shouldn't this use at least ptApprox as expected type? (Or pt with undetparams replaced by their inferred types.)

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 14, 2013

Member
  • Please document the test case a bit with what's being tested.
Member

adriaanm commented May 14, 2013

  • Please document the test case a bit with what's being tested.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 14, 2013

Member
  • one more test case

Also, more convincing examples would help support this PR's case. A common use case would be in database mappers where the companion object is boilerplate.

case class RowA extends MappedRow
implicit object RowA extends RowMapper[RowA]

case class RowB extends MappedRow
implicit object RowB extends RowMapper[RowB]

could be shortened to

case class RowA extends MappedRow
case class RowB extends MappedRow

if the library provides a macro like

implicit macro def mapper[T <: MappedRow]: RowMapper[T] = macro mapperImpl
def mapperImpl = reify { new RowMapper[T] }
Member

adriaanm commented May 14, 2013

  • one more test case

Also, more convincing examples would help support this PR's case. A common use case would be in database mappers where the companion object is boilerplate.

case class RowA extends MappedRow
implicit object RowA extends RowMapper[RowA]

case class RowB extends MappedRow
implicit object RowB extends RowMapper[RowB]

could be shortened to

case class RowA extends MappedRow
case class RowB extends MappedRow

if the library provides a macro like

implicit macro def mapper[T <: MappedRow]: RowMapper[T] = macro mapperImpl
def mapperImpl = reify { new RowMapper[T] }
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 14, 2013

Member

Finally, we should probably reserve new features for 2.11.
2.10.x is the stable release, it's not supposed to get new features anymore.

Member

adriaanm commented May 14, 2013

Finally, we should probably reserve new features for 2.11.
2.10.x is the stable release, it's not supposed to get new features anymore.

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by May 15, 2013

Member

Thank you for the feedback! I'll resubmit once I address it.

Member

xeno-by commented May 15, 2013

Thank you for the feedback! I'll resubmit once I address it.

@xeno-by xeno-by closed this May 15, 2013

xeno-by added a commit to xeno-by/scala that referenced this pull request Dec 27, 2013

[nomaster] codifies the state of the art wrt SI-8104
As it was discovered in SI-8104, whiteboxity doesn’t apply equally to
type parameters and type members of materialized type classes.

During implicit search and subsequent type inference, whitebox type parameters
are consistently erased to wildcards, whereas whitebox type members sometimes
remain as is and get in the way of signature conformance checks.

Unfortunately, 2.10.x can’t make use of type parameter whiteboxity, because
it requires fundep materializers that were only merged into 2.11:
scala#2499, and therefore Si-8104 seems to be
a hard blocker for 2.10.x at the moment. Stay tuned for updates.

xeno-by added a commit to xeno-by/scala that referenced this pull request Dec 28, 2013

[nomaster] codifies the state of the art wrt SI-8104
As it was discovered in SI-8104, whiteboxity doesn’t apply equally to
type parameters and type members of materialized type classes.

During implicit search and subsequent type inference, whitebox type parameters
are consistently erased to wildcards, whereas whitebox type members sometimes
remain as is and get in the way of signature conformance checks.

Unfortunately, 2.10.x can’t make use of type parameter whiteboxity, because
it requires fundep materializers that were only merged into 2.11:
scala#2499, and therefore Si-8104 seems to be
a hard blocker for 2.10.x at the moment. Stay tuned for updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment