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

(2.10.4) whitebox macros are now first typechecked against outerPt #3256

Closed
wants to merge 1 commit into from

Conversation

xeno-by
Copy link
Member

@xeno-by xeno-by commented Dec 10, 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 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.

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.
@ghost ghost assigned retronym Dec 10, 2013
@xeno-by
Copy link
Member Author

xeno-by commented Dec 10, 2013

Backports #3236, review @retronym

@retronym
Copy link
Member

Small process suggestion: I think it is better not to submit the backport PR until the original change is reviewed and fine tuned. I find that having both in queue makes me feel under some pressure to avoid finding faults during review, as I know you'll have to rework the backport.

@xeno-by
Copy link
Member Author

xeno-by commented Dec 11, 2013

@retronym Fair enough, I will keep that in mind in the future. Do you want me to close the backports that are currently in the queue?

@retronym
Copy link
Member

Yes, that would be great. I'm happy that you have them ready, I just want you to keep them to yourself for a little while :)

I suppose it would also be handy to declare your intention to backport in the main PR; maybe the reviewer will develop thoughts on the pros and cons of that while reviewing.

@xeno-by
Copy link
Member Author

xeno-by commented Dec 11, 2013

Closes this PR for now until its master counterpart is merged.

@xeno-by xeno-by closed this Dec 11, 2013
@xeno-by
Copy link
Member Author

xeno-by commented Dec 13, 2013

@adriaanm Given that 2.10.4-RC1 has just been cut, is there a chance that this PR and #3259 are going to make it into 2.10.4? I'd very much like to see them released one of these days, not in three months, especially this one, which is important for scala-blitz.

@adriaanm
Copy link
Contributor

Sorry, the queue was empty from where I was sitting because these were closed.
If there's an RC2, we'll merge them. If not, we'll have to discuss whether this is critical enough to prompt an RC2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants