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

(2.11.0-M8) whitebox macros are now first typechecked against outerPt #3236

Merged
merged 3 commits into from Dec 17, 2013

Conversation

Projects
None yet
5 participants
@xeno-by
Member

xeno-by commented Dec 8, 2013

Even though whitebox macros are supposed to be used to produce expansions
that refine advertised return types of their macro definitions, sometimes
those more precise types aren’t picked up by the typechecker.

It all started with Travis generating structural types with macros
and noticing that typer needs an extra nudge in order to make generated
members accessible to the outside world. I didn’t understand the mechanism
of the phenomenon back then, and after some time I just gave up.

Afterwards, when this issue had been brought up again in a different
StackOverflow question, we discussed it at reflection meeting, figured out
that typedBlock provides some special treatment to anonymous classes,
and it became clear that the first macro typecheck (the one that types
the expansion against the return type of the corresponding macro def)
is at fault here.

The thing is that if we have a block that stands for a desugard anonymous
class instantiation, and we typecheck it with expected type different from
WildcardType, then typer isn’t going to include decls of the anonymous class
in the resulting structural type: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L2350.
I tried to figure it out at https://groups.google.com/forum/#!topic/scala-internals/eXQt-BPm4i8,
but couldn’t dispel the mystery, so again I just gave up.

But today I had a profound WAT experience that finally tipped the scales.
It turns out that if we typecheck an if, providing a suitable pt, then
the resulting type of an if is going to be that pt, even though the lub
of the branch types might be more precise. I’m sure that reasons for this
behavior are also beyond my understanding, so I decided to sidestep this problem.

upd. Here’s Jason’s clarification: Doing thing differently would require
us to believe that "'Tis better to have lubbed and lost than never to have
lubbed at all." But the desire for efficiency trumps such sentimentality.

Now expansions of whitebox macros are first typechecked against outerPt,
the expected type that comes from the enclosing context, before being
typechecked against innerPt, the expected type that comes from the return
type of the macro def. This means that now outerPt provides the correct
expected type for the initial, most important typecheck, which makes
types more precise.

@xeno-by

This comment has been minimized.

Member

xeno-by commented Dec 8, 2013

@ghost ghost assigned retronym Dec 8, 2013

@retronym

This comment has been minimized.

Member

retronym commented Dec 8, 2013

I’m sure that reasons for this behavior are also beyond my understanding, so I decided to sidestep this problem.

Doing thing differently would require us to believe that "'Tis better to have lubbed and lost than never to have lubbed at all." But the desire for efficiency trumps such sentimentality.

@adriaanm

View changes

src/compiler/scala/tools/nsc/typechecker/Macros.scala Outdated
def isBlackbox(macroDef: Symbol): Boolean = loadMacroImplBinding(macroDef).map(_.isBlackbox).getOrElse(false)
def isWhitebox(macroDef: Symbol): Boolean = loadMacroImplBinding(macroDef).map(!_.isBlackbox).getOrElse(false)

This comment has been minimized.

@adriaanm

adriaanm Dec 9, 2013

Member

should this be a tristate instead? third state being "dunno"

This comment has been minimized.

@xeno-by

xeno-by Dec 9, 2013

Member

I agree, duplication here is suboptimal. Therefore I just removed the isWhitebox method, because we actually don't need it.

@xeno-by

This comment has been minimized.

Member

xeno-by commented Dec 10, 2013

@retronym Applied your suggestion, everything works.

@retronym

This comment has been minimized.

Member

retronym commented Dec 10, 2013

Not sure if this matters, but we should consider this sort of thing:

class M[A: ClassTag] { println(implicitly[ClassTag[A]]) }
def impl: Tree = q"new M"
def apply: M[Any] = macro impl
{apply; () }

Under this scheme, we'll infer A = Nothing.

@xeno-by

This comment has been minimized.

Member

xeno-by commented Dec 10, 2013

@retronym Thanks for bringing this up! I think we can live with that.

xeno-by added some commits Dec 10, 2013

refactors macroExpandApply
Moves some code around to clearly define the concepts that the method
operates on: 1) `innerPt`, which is expected type provided by the macro
def return type, 2) `outerPt`, which is expected type provided by the
enclosing context.

Once everything is clearly defined, the gist of the expander fits in
a few lines in its end. If blackbox, do this. If whitebox, do that.

Note that unlike the subsequent commit, this commit doesn’t change
the way how macro expansion works. It just clears everything out, so that
the upcoming changes can be applied in a concise and comprehensible manner.
whitebox macros are now first typechecked against outerPt
Even though whitebox macros are supposed to be used to produce expansions
that refine advertised return types of their macro definitions, sometimes
those more precise types aren’t picked up by the typechecker.

It all started with Travis generating structural types with macros
and noticing that typer needs an extra nudge in order to make generated
members accessible to the outside world. I didn’t understand the mechanism
of the phenomenon back then, and after some time I just gave up.

Afterwards, when this issue had been brought up again in a different
StackOverflow question, we discussed it at reflection meeting, figured out
that typedBlock provides some special treatment to anonymous classes,
and it became clear that the first macro typecheck (the one that types
the expansion against the return type of the corresponding macro def)
is at fault here.

The thing is that if we have a block that stands for a desugard anonymous
class instantiation, and we typecheck it with expected type different from
WildcardType, then typer isn’t going to include decls of the anonymous class
in the resulting structural type: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L2350.
I tried to figure it out at https://groups.google.com/forum/#!topic/scala-internals/eXQt-BPm4i8,
but couldn’t dispel the mystery, so again I just gave up.

But today I had a profound WAT experience that finally tipped the scales.
It turns out that if we typecheck an if, providing a suitable pt, then
the resulting type of an if is going to be that pt, even though the lub
of the branch types might be more precise. I’m sure that reasons for this
behavior are also beyond my understanding, so I decided to sidestep this problem.

upd. Here’s Jason’s clarification: Doing thing differently would require
us to believe that "'Tis better to have lubbed and lost than never to have
lubbed at all." But the desire for efficiency trumps such sentimentality.

Now expansions of whitebox macros are first typechecked against outerPt,
the expected type that comes from the enclosing context, before being
typechecked against innerPt, the expected type that comes from the return
type of the macro def. This means that now outerPt provides the correct
expected type for the initial, most important typecheck, which makes
types more precise.
drops the redundant typecheck of blackbox expansions
While fixing the problem with the order of typechecks for whitebox expansions,
I realized that we’re doing redundant work when expanding blackbox macros.
Concretely, typechecking blackbox expansions looked as follows:

  val expanded1 = atPos(enclosingMacroPosition.focus)(Typed(expanded0, TypeTree(innerPt)))
  val expanded2 = typecheck("blackbox typecheck #1", expanded1, innerPt)
  typecheck("blackbox typecheck #2", expanded1, outerPt)

Or, if we reformulate it using quasiquotes (temporarily not taking
positions into account, since they aren’t important here):

  val expanded2 = typed(q”$expanded: $innerPt”, innerPt)
  typed(expanded2, outerPt)

In this formulation, it becomes apparent that the first typecheck is
redundant. If something is ascribed with some type, then typechecking
the ascription against that type does nothing useful.

This is also highlights one of the reasons why it would be really nice
to have quasiquotes used in the compiler. With them, it’s easy to notice
things that would otherwise remain buried behind swaths of boilerplate.
@paulp

This comment has been minimized.

Contributor

paulp commented Dec 17, 2013

@xeno-by @retronym

It turns out that if we typecheck an if, providing a suitable pt, then the resulting type of an if is going to be that pt, even though the lub of the branch types might be more precise. I’m sure that reasons for this behavior are also beyond my understanding, so I decided to sidestep this problem.

Isn't he talking about the special case hacks in typedIf? The comment doesn't give much indication it's about performance, at least not primarily. It sounds more like bug avoidance shenanigans.

// in principle we should pack the types of each branch before lubbing, but lub doesn't really work for existentials anyway
// in the special (though common) case where the types are equal, it pays to pack before comparing
// especially virtpatmat needs more aggressive unification of skolemized types
// this breaks src/library/scala/collection/immutable/TrieIterator.scala
// annotated types need to be lubbed regardless (at least, continations break if you by pass them like this)
@retronym

This comment has been minimized.

Member

retronym commented Dec 17, 2013

Not quite: it's this part that avoids lubbing the branches:

def finish(ownType: Type) = treeCopy.If(tree, cond1, thenp1, elsep1) setType ownType
if (isFullyDefined(pt))
  finish(pt)

So, in (if (true) "" else ""): Any, the If has type Any, rather than String.

@retronym

This comment has been minimized.

Member

retronym commented Dec 17, 2013

LGTM

xeno-by added a commit that referenced this pull request Dec 17, 2013

Merge pull request #3236 from xeno-by/topic/wildbox-macros
(2.11.0-M8) whitebox macros are now first typechecked against outerPt

@xeno-by xeno-by merged commit b5ef79f into scala:master Dec 17, 2013

1 check passed

default pr-scala Took 81 min.
Details

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

[nomaster] backports scala#3236
Even though whitebox macros are supposed to be used to produce expansions
that refine advertised return types of their macro definitions, sometimes
those more precise types aren’t picked up by the typechecker.

It all started with Travis generating structural types with macros
and noticing that typer needs an extra nudge in order to make generated
members accessible to the outside world. I didn’t understand the mechanism
of the phenomenon back then, and after some time I just gave up.

Afterwards, when this issue had been brought up again in a different
StackOverflow question, we discussed it at reflection meeting, figured out
that typedBlock provides some special treatment to anonymous classes,
and it became clear that the first macro typecheck (the one that types
the expansion against the return type of the corresponding macro def)
is at fault here.

The thing is that if we have a block that stands for a desugard anonymous
class instantiation, and we typecheck it with expected type different from
WildcardType, then typer isn’t going to include decls of the anonymous class
in the resulting structural type: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L2350.
I tried to figure it out at https://groups.google.com/forum/#!topic/scala-internals/eXQt-BPm4i8,
but couldn’t dispel the mystery, so again I just gave up.

But today I had a profound WAT experience that finally tipped the scales.
It turns out that if we typecheck an if, providing a suitable pt, then
the resulting type of an if is going to be that pt, even though the lub
of the branch types might be more precise. I’m sure that reasons for this
behavior are also beyond my understanding, so I decided to sidestep this problem.

upd. Here’s Jason’s clarification: Doing thing differently would require
us to believe that "'Tis better to have lubbed and lost than never to have
lubbed at all." But the desire for efficiency trumps such sentimentality.

Now expansions of whitebox macros are first typechecked against pt,
the expected type that comes from the enclosing context, before being
typechecked against expectedTpe, the expected type that comes from the return
type of the macro def. This means that now pt provides the correct
expected type for the initial, most important typecheck, which makes
types more precise.

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

[nomaster] backports scala#3236
Even though whitebox macros are supposed to be used to produce expansions
that refine advertised return types of their macro definitions, sometimes
those more precise types aren’t picked up by the typechecker.

It all started with Travis generating structural types with macros
and noticing that typer needs an extra nudge in order to make generated
members accessible to the outside world. I didn’t understand the mechanism
of the phenomenon back then, and after some time I just gave up.

Afterwards, when this issue had been brought up again in a different
StackOverflow question, we discussed it at reflection meeting, figured out
that typedBlock provides some special treatment to anonymous classes,
and it became clear that the first macro typecheck (the one that types
the expansion against the return type of the corresponding macro def)
is at fault here.

The thing is that if we have a block that stands for a desugard anonymous
class instantiation, and we typecheck it with expected type different from
WildcardType, then typer isn’t going to include decls of the anonymous class
in the resulting structural type: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L2350.
I tried to figure it out at https://groups.google.com/forum/#!topic/scala-internals/eXQt-BPm4i8,
but couldn’t dispel the mystery, so again I just gave up.

But today I had a profound WAT experience that finally tipped the scales.
It turns out that if we typecheck an if, providing a suitable pt, then
the resulting type of an if is going to be that pt, even though the lub
of the branch types might be more precise. I’m sure that reasons for this
behavior are also beyond my understanding, so I decided to sidestep this problem.

upd. Here’s Jason’s clarification: Doing thing differently would require
us to believe that "'Tis better to have lubbed and lost than never to have
lubbed at all." But the desire for efficiency trumps such sentimentality.

Now expansions of whitebox macros are first typechecked against pt,
the expected type that comes from the enclosing context, before being
typechecked against expectedTpe, the expected type that comes from the return
type of the macro def. This means that now pt provides the correct
expected type for the initial, most important typecheck, which makes
types more precise.

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

[nomaster] backports scala#3236
Even though whitebox macros are supposed to be used to produce expansions
that refine advertised return types of their macro definitions, sometimes
those more precise types aren’t picked up by the typechecker.

It all started with Travis generating structural types with macros
and noticing that typer needs an extra nudge in order to make generated
members accessible to the outside world. I didn’t understand the mechanism
of the phenomenon back then, and after some time I just gave up.

Afterwards, when this issue had been brought up again in a different
StackOverflow question, we discussed it at reflection meeting, figured out
that typedBlock provides some special treatment to anonymous classes,
and it became clear that the first macro typecheck (the one that types
the expansion against the return type of the corresponding macro def)
is at fault here.

The thing is that if we have a block that stands for a desugard anonymous
class instantiation, and we typecheck it with expected type different from
WildcardType, then typer isn’t going to include decls of the anonymous class
in the resulting structural type: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L2350.
I tried to figure it out at https://groups.google.com/forum/#!topic/scala-internals/eXQt-BPm4i8,
but couldn’t dispel the mystery, so again I just gave up.

But today I had a profound WAT experience that finally tipped the scales.
It turns out that if we typecheck an if, providing a suitable pt, then
the resulting type of an if is going to be that pt, even though the lub
of the branch types might be more precise. I’m sure that reasons for this
behavior are also beyond my understanding, so I decided to sidestep this problem.

upd. Here’s Jason’s clarification: Doing thing differently would require
us to believe that "'Tis better to have lubbed and lost than never to have
lubbed at all." But the desire for efficiency trumps such sentimentality.

Now expansions of whitebox macros are first typechecked against pt,
the expected type that comes from the enclosing context, before being
typechecked against expectedTpe, the expected type that comes from the return
type of the macro def. This means that now pt provides the correct
expected type for the initial, most important typecheck, which makes
types more precise.
@adriaanm

This comment has been minimized.

Member

adriaanm commented on ca2dbe5 Dec 18, 2013

actually, this seems to have cause regression https://issues.scala-lang.org/browse/SI-8091

This comment has been minimized.

Member

xeno-by replied Dec 18, 2013

I see.

@jvican

This comment has been minimized.

Member

jvican commented May 23, 2018

@retronym Is it on your roadmap to look at the root issues here so that we can avoid the double typechecking of whitebox macros?

@retronym

This comment has been minimized.

Member

retronym commented May 23, 2018

That's not on my radar. Is there a canonical example of the problem?

@jvican

This comment has been minimized.

Member

jvican commented May 24, 2018

The problem regarding lubs is the responsible for the reason why we typecheck whitebox macros three times instead of two, so I was wondering if it was worth having a look at it again so that we could get rid of the extra typechecker. Ideally, we would only typecheck whitebox macros once (as blackbox macros), but I don't know if that's technically feasible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment