RootPackage symbols gets LATEMETHOD flag

[Eugene Burmako]

What's going on? Is this intended?

[Paul Phillips]

On Fri, Aug 10, 2012 at 12:16 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

What's going on? Is this intended?

Which symbols specifically?

[Eugene Burmako]

Sorry, that was a typo. I meant the RootPackage symbol.

This one: final object RootPackage extends ModuleSymbol(rootOwner, NoPosition, nme.ROOTPKG)

[Paul Phillips]

On Fri, Aug 10, 2012 at 12:48 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Sorry, that was a typo. I meant the RootPackage symbol.

This one: final object RootPackage extends ModuleSymbol(rootOwner, NoPosition, nme.ROOTPKG)

What makes you think it's getting lateMETHOD? I don't see it.

[Eugene Burmako]

At lambdalift it has the following flags:

scala> ru.show(9024791446044960L.asInstanceOf[FlagSet])
res0 @ 44157e43: String = FINAL | MODULE | PACKAGE | JAVA | STABLE | TRIEDCOOKING | LATEMETHOD

From a quick experiment with -Xprint, it seems that it becomes a "method" in uncurry. Before that it's printed as _root_, but with -Xprint:uncurry it's printed as _root_() [1]

[1] https://gist.github.com/3317557

[Paul Phillips]

On Fri, Aug 10, 2012 at 1:23 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

[1] https://gist.github.com/3317557

Where is the source code?

[Eugene Burmako]

https://gist.github.com/3317596

[Paul Phillips]

Can you incur "_root_()" without any xml? I think it's a bug in the xml parser.

[Eugene Burmako]

Also see Hubert's comments to https://issues.scala-lang.org/browse/SI-6201

The problem is that root package qualifier from root.scala.xml.Null somehow gets past Typer and later we crash.
adaptToMemberWithArgs is a bit at fault but the underlying cause may be somewhere else.

[Hubert Plociniczak]

On Aug 10, 2012, at 10:33 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Also see Hubert's comments to https://issues.scala-lang.org/browse/SI-6201

The problem is that root package qualifier from root.scala.xml.Null somehow gets past Typer and later we crash.

 That's only partially the reason. I only later noticed that we don't (or least weren't in the past) removing the qualifier consistently. It's somewhere in Typers ( not on my laptop atm so cannot give you the location).

Since I never really touched that part I don't know if it is intended or not.

[Eugene Burmako]

Okay, RootPackage gets its lateMETHOD flag because of this code in refchecks:

override def transformInfo(sym: Symbol, tp: Type): Type = {
  if (sym.isModule && !sym.isStatic) sym setFlag (lateMETHOD | STABLE)
  super.transformInfo(sym, tp)
}

[Paul Phillips]

On Fri, Aug 10, 2012 at 2:15 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Okay, RootPackage gets its lateMETHOD flag because of this code in refchecks:

override def transformInfo(sym: Symbol, tp: Type): Type = {
  if (sym.isModule && !sym.isStatic) sym setFlag (lateMETHOD | STABLE)
  super.transformInfo(sym, tp)
}

Yes, that is the only place anything gets its lateMETHOD flag.

[Paul Phillips]

I don't know what changed, but the simple solution here is to fix isStatic so it is not willing to claim that packages are not static.

 - def isStatic = (this hasFlag STATIC) || owner.isStaticOwner

 + def isStatic = (this hasFlag STATIC | PACKAGE) || owner.isStaticOwner

[Eugene Burmako]

Right. Or we could alternatively override isStatic in root symbols (which I think would be a good idea anyways).

But I want to get to the bottom of it. It seems that typer after refactoring doesn't strip off RootPackage qualifiers as in "if (qual1.symbol == RootPackage) treeCopy.Ident(tree1, name) else tree1". Weird.

[Hubert Plociniczak]

On Aug 10, 2012, at 11:39 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Right. Or we could alternatively override isStatic in root symbols (which I think would be a good idea anyways).

But I want to get to the bottom of it. It seems that typer after refactoring doesn't strip off RootPackage qualifiers as in "if (qual1.symbol == RootPackage) treeCopy.Ident(tree1, name) else tree1". Weird.

It's because of context.tree in adaptToMember...

[Eugene Burmako]

Okay found it. `Tree.hasSymbolWhich` is implemented incorrectly. The offending patch adds a check for this.hasSymbol, which is false for Apply nodes. Hence _root_ qualifier doesn't get stripped off, and bad things happen.

[Eugene Burmako]

Funny! It took a superposition of three oversights (adaptToMember not removing _root_, _root_ having isStatic set to false and broken hasSymbolWhich) to blow up. The first two oversights alone were not enough :)

[Paul Phillips]

On Fri, Aug 10, 2012 at 2:51 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Okay found it. `Tree.hasSymbolWhich` is implemented incorrectly. The offending patch adds a check for this.hasSymbol, which is false for Apply nodes. Hence _root_ qualifier doesn't get stripped off, and bad things happen.

I'm glad I could provide the critical third leg of the bug platform.

I see hasSymbol can just be tossed from hasSymbolWhich entirely.  It exists primarily to avoid all the NPEs one enjoys when depending on everyone to check .symbol != null every time.

[Eugene Burmako]

Here's the patch: https://github.com/scala/scala/pull/1118. 

I know next to nothing about adaptToMember, so I only fixed two legs of three.

[martin odersky]

On Fri, Aug 10, 2012 at 11:15 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Okay, RootPackage gets its lateMETHOD flag because of this code in refchecks:

override def transformInfo(sym: Symbol, tp: Type): Type = {
  if (sym.isModule && !sym.isStatic) sym setFlag (lateMETHOD | STABLE)
  super.transformInfo(sym, tp)
}

Clearly, root package needs to be static, so that's a bug. I am less sure about hasSymbolWhich. I think it is correct as defined. If a tree does not have a symbol (ie hasSymbol is false) then clearly hasSymbolWhich should also be false. Clients need to take that into account.

Cheers

 - Martin

On 10 August 2012 21:48, Eugene Burmako <eugene....@epfl.ch> wrote:

Sorry, that was a typo. I meant the RootPackage symbol.

This one: final object RootPackage extends ModuleSymbol(rootOwner, NoPosition, nme.ROOTPKG)

On 10 August 2012 21:40, Paul Phillips <pa...@improving.org> wrote:

On Fri, Aug 10, 2012 at 12:16 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

What's going on? Is this intended?

Which symbols specifically?

--
Martin Odersky
Prof., EPFL and Chairman, Typesafe
PSED, 1015 Lausanne, Switzerland
Tel. EPFL: +41 21 693 6863
Tel. Typesafe: +41 21 691 4967

[Eugene Burmako]

Apply.hasSymbol is false, but it does have a symbol, because it overrides the symbol getter with fun.symbol. 

So either Apply.hasSymbol is incorrect, or we should allow for Apply as it has been done before.

[Paul Phillips]

On Sun, Aug 12, 2012 at 4:09 AM, martin odersky <martin....@epfl.ch> wrote:

If a tree does not have a symbol (ie hasSymbol is false) then clearly hasSymbolWhich should also be false. Clients need to take that into account.

The problem is that what it means to "have a symbol" is ill-defined.  "hasSymbol" would be better called "hasSymbolField" because that's what the implementation is really testing.  However "hasSymbolWhich" is intended for the generally far more useful question of what happens when you call .symbol on that tree (but avoiding NPEs.) The difference arises in the trees which forward the symbol call to some underlying tree, as Apply/TypeApply do to their first children.

[martin odersky]

On Sun, Aug 12, 2012 at 1:14 PM, Eugene Burmako <eugene....@epfl.ch> wrote:

Apply.hasSymbol is false, but it does have a symbol, because it overrides the symbol getter with fun.symbol. 

So either Apply.hasSymbol is incorrect, or we should allow for Apply as it has been done before.

You should allow for Apply. It's not just Apply but also TypeApply. Essentillay, hasSymbol means: Tree defines a Symbol. Apply, and TypeApply don't; they just forward.

Cheers

 - Martin