Skip to content

SI-8947 Avoid cross talk between tag materializers and reify#4083

Merged
retronym merged 2 commits intoscala:2.11.xfrom
retronym:ticket/8947
Nov 7, 2014
Merged

SI-8947 Avoid cross talk between tag materializers and reify#4083
retronym merged 2 commits intoscala:2.11.xfrom
retronym:ticket/8947

Conversation

@retronym
Copy link
Member

After a macro has been expanded, the expandee are expansion are
bidirectionally linked with tree attachments. Reify uses the back
reference to replace the expansion with the expandee in the reified
tree. It also has some special cases to replace calls to macros
defined in scala-compiler.jar with Predef.implicitly[XxxTag[T]].
This logic lives in Reshape.

However, the expansion of a macro may be EmptyTree. This is the case
when a tag materializer macro fails. User defined macros could do the
also expand to EmptyTree. In the enclosed test case, the error
message that the tag materializer issued ("cannot materialize
class tag for unsplicable type") is not displayed as the typechecker
finds another means of making the surrounding expression typecheck.

However, the macro engine attaches a backreference to the materializer
macro on EmpytyTree!

Later, when reify reshapes a tree, every occurance of EmptyTree
will be replaced by a call to implicitly.

This commit expands the domain of CannotHaveAttrs, which is mixed
in to EmptyTree. It silently ignores all attempts to mutate
attachments.

Unlike similar code that discards mutations of its type and position,
I have refrained from issuing a developer warning in this case, as
to silence this I would need to go and add a special case at any
places adding attachments.

Review by @xeno-by @densh

After a macro has been expanded, the expandee are expansion are
bidirectionally linked with tree attachments. Reify uses the back
reference to replace the expansion with the expandee in the reified
tree. It also has some special cases to replace calls to macros
defined in scala-compiler.jar with `Predef.implicitly[XxxTag[T]]`.
This logic lives in `Reshape`.

However, the expansion of a macro may be `EmptyTree`. This is the case
when a tag materializer macro fails. User defined macros could do the
also expand to `EmptyTree`. In the enclosed test case, the error
message that the tag materializer issued ("cannot materialize
class tag for unsplicable type") is not displayed as the typechecker
finds another means of making the surrounding expression typecheck.

However, the macro engine attaches a backreference to the materializer
macro on `EmpytyTree`!

Later, when `reify` reshapes a tree, every occurance of `EmptyTree`
will be replaced by a call to `implicitly`.

This commit expands the domain of `CannotHaveAttrs`, which is mixed
in to `EmptyTree`. It silently ignores all attempts to mutate
attachments.

Unlike similar code that discards mutations of its type and position,
I have refrained from issuing a developer warning in this case, as
to silence this I would need to go and add a special case at any
places adding attachments.
@scala-jenkins scala-jenkins added this to the 2.11.5 milestone Oct 30, 2014
@xeno-by
Copy link
Contributor

xeno-by commented Oct 30, 2014

Thanks for the explanation! I've been indeed able to track EmptyTree to

def materializeExpr(universe: Tree, mirror: Tree, expr: Tree): Tree = {
. An unexpected fact is that c.typecheck(EmptyTree) actually succeeds, which skips c.abort.

My suggestion is three-fold:

  1. How about we check for EmptyTree over there and just call c.abort if we encounter it? That would fix the problem at its root.
  2. Additionally, we could safeguard the line of code that does the attachment (https://github.com/scala/scala/blob/2.11.x/src/compiler/scala/tools/nsc/typechecker/Macros.scala#L619) by checking for CannotHaveAttrs.
  3. Now, we could have attachment-related functionality in CannotHaveAttrs throw exceptions.

What do you think?

@retronym
Copy link
Member Author

I like all of that, but I would prefer dev warnings to an error, as we do elsewhere in cannothaveattrs. Could I ask you to submit that?

@retronym
Copy link
Member Author

retronym commented Nov 6, 2014

@xeno-by I've added a commit with your suggestions. Could you please review?

As suggested in review:

  - Use `abort` rather than `{error; EmptyTree} when we hit an
    error in reification or tag materialization.
  - Explicitly avoid adding the `MacroExpansionAttachment` to the
    macro expansion if it an `EmptyTree`
  - Emit a `-Xdev` warning if any other code paths find a way to
    mutate attachments in places they shouldn't.
@xeno-by
Copy link
Contributor

xeno-by commented Nov 6, 2014

LGTM

@retronym
Copy link
Member Author

retronym commented Nov 6, 2014

PLS REBUILD ALL

@scala-jenkins
Copy link

(kitty-note-to-self: ignore 61979143)
🐱 Roger! Rebuilding pr-scala for d056439, 17992f6. 🚨

retronym added a commit that referenced this pull request Nov 7, 2014
SI-8947 Avoid cross talk between tag materializers and reify
@retronym retronym merged commit 7830b46 into scala:2.11.x Nov 7, 2014
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 this pull request may close these issues.

3 participants