resetAttrs now always erases This.tpe

[xeno-by]

The symbol of This, if it points to a package class, isn't touched, just
as usual, so that our Select(Select(Select(...))) => This(...) optimization
works fine with attr reset.

However the tpe is now erased, so that subsequent reflective compilation
doesn't spuriously fail when seeing that some subtrees of a tree being
compiled are typed.

Erasing the tpe doesn't pose even a tiniest problem, because, as it can
be seen in typedThis, type is trivially reconstructed from the symbol.

[xeno-by]

review @retronym @paulp

[paulp]

I can't say a patch such as this looks good to me; I don't know how anyone can be very confident of the before/after behavior. Imagine we were tasked with convincing someone not raised from birth on compiler arcanery of the correctness of this change. How would we go about that?

Maybe you could at least add a comment explaining what is happening with the "vetoscope/vetolabel/vetothis" calculations.

[lrytz]

for starters, the patch fixes an obvious contract violation of the typer: he's not supposed to a) set the "tpe" field of trees that are passed into it, b) return the same tree that's passed into it, and c) return a tree which is partially typed (subtrees of the returned tree have tpe / symbol still null).

[lrytz]

actually, b) is not true if the input tree is already typed.

[paulp]

"Well," says the hypothetical person we're trying to convince, "I looked into how contractual this contract is."

    def typed(tree: Tree, mode: Mode, pt: Type): Tree = {
      val untyped = tree.tpe eq null
      val res = typedInternal(tree, mode, pt)
      if (untyped && (tree eq res))
        println(s"typed mutates in-place: $tree".takeWhile(_ != '\n'))

      res
    }

And here are the results compiling only the library and stopping after phase typer in the interests of mercy.

% qscalac -d /tmp -Ystop-after:typer -J-Xmx2g -d /tmp $(find ./src/library -name '*.scala') | wc -l
  144505
%

I suppose this is what I'm driving at when I say I don't see how anyone can have much confidence about whether such a change is correct. What makes this an obvious violation of the typer contract? For it to be an obvious violation of the typer contract we should probably share some common idea of what the typer contract is. Is it written down somewhere, or is it a strictly verbal tradition? The location one might predict, 'def typed', doesn't even arrive bearing a comment.

[lrytz]

uh, paul i'm sorry, i confused two pull requests, i thought your comment was on #2118 - my answer doesn't make much sense here...

[xeno-by]

added comments, fixed something we've supposedly overlooked in resetAttrs for TypeTree. /cc @retronym

[xeno-by]

PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 13774338)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for e2a17d9c, bb2c0475, baec766d. 🚨

[xeno-by]

oops the "fix" was incorrect. rolled it back.

[paulp]

@lrytz I realized it must be something like that when I looked at the commit again and couldn't find the referents. Still, it would be great to get some answers to those questions about the typer contract regardless of their applicability here.

[retronym]

lgtm

We now can say:

resetAttrs(tree).forall(_.tpe == null) "reset trees are completely untyped, although they may retain some defined symbols"

Other tree invariants?

(tree.tpe != null) → tree.forall(_.tpe != null) "typed trees can't have untyped children"

[xeno-by]

Actually "reset trees are completely untyped, although they may retain some defined symbols" is incorrect. Some type trees aren't erased. See the last commit. PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 13778952)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for e2a17d9c, bb2c0475, ec792ad5. 🚨

[xeno-by]

PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 13796627)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for e2a17d9c, bb2c0475, 2c568222. 🚨

[xeno-by]

Shall we merge?

[JamesIry]

@retronym you gave the lgtm above, but there have been commits since then. Do you want to take a second look?

[xeno-by]

I only updated the comment, so the review shouldn't be hard I think.

[xeno-by]

Oh actually that wasn't just a comment. Sorry

[retronym]

refersToLocalSymbols leaves less room for confusion; "erasure" is too synonymous with the type erasure phase.

[paulp]

Capitalization, periods, and whitespace are your tools.

You are my hero.

P.S. There are also tools which are your tools. However in this case they mostly serve to illustrate why periods and whitespace are still valuable in 2013.

// typically the resetAttrs transformer cleans both symbols and types
// however there are exceptions when we cannot erase symbols due to
// idiosynchrazies of the typer vetoXXX local variables declared below
// describe the conditions under which we cannot erase symbols 1) the
// first reason to not erase symbols is the threat of non-idempotency
// (SI-5464) here we take care of labels (SI-5562) and references to
// package classes (SI-5705) there are other non-idempotencies, but
// they are not worked around yet 2) the second reason has to do with
// the fact that resetAttrs itself has limited usefulness first of
// all, why do we need resetAttrs? for one, it's absolutely required
// to move trees around one cannot just take a typed tree from one
// lexical context and transplant it somewhere else most likely symbols
// defined by those trees will become borked and the compiler will blow
// up (SI-5797) to work around we just erase all symbols and types and
// then hope that we'll be able to correctly retypecheck for ones who're
// not affected by scalac Stockholm syndrome, this might seem to be an
// extremely naive fix, but well of course, sometimes erasing everything
// won't work, because if a given identifier got resolved to something
// in one lexical scope, it can get resolved to something else. what
// do we do in these cases? enter the workaround for the workaround,
// resetLocalAttrs, which only destroys locally defined symbols, but
// doesn't touch references to stuff declared outside of a given tree.
// that's what localOnly and vetoScope are for

[xeno-by]

PLS REBUILD ALL

[adriaanm]

Note that I consider it a bug when you have to ask the kitten to rebuild explicitly. It won't speed things up in the current design.

[adriaanm]

(Since it builds commits that are not built yet as they are pushed to the PR's branch.)

[scala-jenkins]

(kitty-note-to-self: ignore 14131850)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for e2a17d9c, 049e7cbf. 🚨

[xeno-by]

thank you, Adriaan!

[retronym]

s/idiosynchrazies/idiosynchracies/ and we're done. Thanks for the prettification.

[xeno-by]

done

[paulp]

Thanks.