SI-8947 Avoid cross talk between tag materializers and reify#4083
Merged
retronym merged 2 commits intoscala:2.11.xfrom Nov 7, 2014
Merged
SI-8947 Avoid cross talk between tag materializers and reify#4083retronym merged 2 commits intoscala:2.11.xfrom
retronym merged 2 commits intoscala:2.11.xfrom
Conversation
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.
Contributor
|
Thanks for the explanation! I've been indeed able to track c.typecheck(EmptyTree) actually succeeds, which skips c.abort.
My suggestion is three-fold:
What do you think? |
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? |
Member
Author
|
@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.
Contributor
|
LGTM |
Member
Author
|
PLS REBUILD ALL |
retronym
added a commit
that referenced
this pull request
Nov 7, 2014
SI-8947 Avoid cross talk between tag materializers and reify
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 casewhen a tag materializer macro fails. User defined macros could do the
also expand to
EmptyTree. In the enclosed test case, the errormessage 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
reifyreshapes a tree, every occurance ofEmptyTreewill be replaced by a call to
implicitly.This commit expands the domain of
CannotHaveAttrs, which is mixedin to
EmptyTree. It silently ignores all attempts to mutateattachments.
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