Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SI-6648 copyAttrs must preserve TypeTree#wasEmpty #1615

Merged
merged 1 commit into from Nov 16, 2012

Conversation

Projects
None yet
7 participants
Owner

retronym commented Nov 13, 2012

This field tracks whether the type is an inferred
one, subject to removal in resetAttrs, or an explicit
type, which must remain.

In ae5ff66, ResetAttrs was modified to duplicate
trees, rather than mutate trees in place. But the
tree copier didn't pass wasEmpty on to the new tree,
which in turn meant that the subsequent typing run
on the tree would not re-infer the types. If the
type refers to a local class, e.g. the anonymous
function in the enclosed test case, the reference
to the old symbol would persist.

This commit overrides copyAttrs in TypeTree to
copy wasEmpty.

We might consider representing this as a tree
attachment, but this would need to be validated
for the performance impact.

Review by @lrytz

Member

hubertp commented Nov 13, 2012

Nice catch!

Owner

retronym commented Nov 13, 2012

How I found it: add val origin = Thread.currentThread.stackTrace to Tree, and then at the spot where the polluted tree shows up, that stack trace implicated TreeCopier.

Contributor

paulp commented Nov 13, 2012

Have you discovered "Origins" yet? You might like it. Given a method like

def f = { ... }

You can add only this:

def f = Origins("f") { ... }

And at shutdown you will be given a detailed report about who called it. A second integer argument to Origins specifies the number of stack frames to track.

Owner

retronym commented Nov 13, 2012

My val was named in honour of it. In this case I needed to track down the origin of one misbehaved tree.

Contributor

paulp commented Nov 13, 2012

In this case I needed to track down the origin of one misbehaved tree.

You can use it selectively too:

if (tree.isMisbehaving)
  Origins("ain't misbehavin'")(tree)
Contributor

paulp commented Nov 13, 2012

This looks good to me by the way. Real purty.

Member

xeno-by commented Nov 13, 2012

Wow this Origins thing is cool. Thank you

Owner

lrytz commented Nov 13, 2012

👏 applause, lgtm

Owner

adriaanm commented Nov 14, 2012

and it even comes with a Dylan reference

Owner

adriaanm commented Nov 14, 2012

could you amend the commit message to reference SI-6648 please?

I get this doubling of the wrong letter all the time with my own name so I know how SI-6648 must feel when it gets called SI-6448

@retronym retronym SI-6648 copyAttrs must preserve TypeTree#wasEmpty
This field tracks whether the type is an inferred
on, subject to removal in `resetAttrs`, or an explicit
type, which must remain.

In ae5ff66, `ResetAttrs` was modified to duplicate
trees, rather than mutate trees in place. But the
tree copier didn't pass `wasEmpty` on to the new tree,
which in turn meant that the subsequent typing run
on the tree would not re-infer the types. If the
type refers to a local class, e.g. the anonymous
function in the enclosed test case, the reference
to the old symbol would persist.

This commit overrides `copyAttrs` in TypeTree to
copy `wasEmpty`.

We might consider representing this as a tree
attachment, but this would need to be validated
for the performance impact.
1587a77
Owner

retronym commented Nov 14, 2012

Done.

@adriaanm adriaanm added a commit that referenced this pull request Nov 16, 2012

@adriaanm adriaanm Merge pull request #1615 from retronym/ticket/6648
SI-6648 copyAttrs must preserve TypeTree#wasEmpty
83deccf

@adriaanm adriaanm merged commit 83deccf into scala:2.10.0-wip Nov 16, 2012

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