SI-8263 Avoid SOE in logicallyEnclosingMember

[retronym]

Not sure when the regression hit, but we're seeing this in the
SBT build. It's probably a mistake to be calling this method
on somthing that isn't owned by a method, but let's not SOE
in that case.

This is analagous with the way the NoSymbol#owner doesnt throw
and exception anymore.

Review by @gkossakowski

[gkossakowski]

The fde88c7 introduced some loggging behind -Xdev option. Does it make sense to do the same in this case?

[retronym]

Yeah, that would be consistent. I've also found the root cause: a macro SBT creates a symbol owned by NoSymbol. I'll submit that fix to SBT, but I think we should also merge this patch (once amended with your suggestion)

[gkossakowski]

If the problem is on sbt side then why it doesn't crash on 2.10.x?

W dniu środa, 12 lutego 2014 Jason Zaugg notifications@github.com
napisał(a):

Yeah, that would be consistent. I've also found the root cause: a macro
SBT creates a symbol owned by NoSymbol. I'll submit that fix to SBT, but
I think we should also merge this patch (once amended with your suggestion)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3506#issuecomment-34860609
.

Grzegorz Kossakowski
Scalac hacker at Typesafe http://www.typesafe.com/
twitter: @gkossakowski http://twitter.com/gkossakowski
github: @gkossakowski http://github.com/gkossakowski

[retronym]

See: sbt/sbt#1113

[retronym]

If the problem is on sbt side then why it doesn't crash on 2.10.x?

We uncovered the problem with the fix for SI-2066.

[retronym]

@gkossakowski I've also updated this PR with a more detailed commit comment and a -Xdev warning.

[gkossakowski]

After thinking about it some more I wonder if fde88c7 and 555db25 are going in the right direction. Asking anything about NoSymbol makes no sense so why not just fail fast?

In particular, SOE exhibited a real bug in sbt. If this patch was merged we wouldn't see the bug or it would fail in some other, more distant (from macro expansion logic) place. I don't see that as an improvement.

I might have expressed that concern around the time fde88c7 was merged but I can't find the outcome of our discussion.

[retronym]

It is at least consistent with:

    override def isHigherOrderTypeParameter = false
    override def companionClass = NoSymbol
    override def companionModule = NoSymbol
    override def companionSymbol = NoSymbol
    override def isSubClass(that: Symbol) = false
    override def filter(cond: Symbol => Boolean) = this
    override def defString: String = toString
    override def locationString: String = ""
    override def enclClassChain = Nil
    override def enclClass: Symbol = this
    override def enclosingTopLevelClass: Symbol = this
    override def enclosingPackageClass: Symbol = this
    override def enclMethod: Symbol = this

owner was the odd man out.

[gkossakowski]

Fair enough. I think I'll want to revisit this design choice in 2.12.

[gkossakowski]

LGTM.

I'll merge once tests pass.