Fix #241

[odersky]

Finally works now. Note that this needs the previous fixes to go in as well. Only two commits are new here. I tried on master, but this caused many errors which go away when using branch fix-#939.

Review by @smarter or @samuelgruetter (only last two commits).

[smarter]

OK, I'm not exactly sure what the proper fix is but:

1. In ClassInfo#typeRef, if this is ClassInfo(Mix___eFoo_I_wBar__f, class I), then the owner of the class I is Foo_I_ which is different from this.prefix, so clsDenot return cls.denot.copySymDenotation(info = this).
2. In NamedType#computeDenot we call prefix.isTightPrefix(d.owner) to check if we need to recompute the denotation of this type. The prefix of the TypeRef computed by the method in step 1 is Mix___eFoo_I_wBar__f, but the owner is Foo_I_, so we end up recomputing the denot, which leads to subtyping checks, which leads to a loop.

Since we don't allow class overriding in Dotty I think we can just bypass the isTightPrefix check when the denotation is a ClassDenotation like this: https://github.com/smarter/dotty/commits/fix-%23241

Alternatively, we could change the owner of the denot returned by clsDenot in ClassInfo#typeRef so that isTightPrefix return true.

[odersky]

In general we can't prevent the recomputation of the denotation without
giving it a fixed sym. That's what symbolicTypeRef does.

On Mon, Dec 14, 2015 at 10:45 PM, Guillaume Martres <
[email protected]> wrote:

OK, I'm not exactly sure what the proper fix is but:

1. In ClassInfo#typeRef, if this is ClassInfo(Mix___eFoo_I_wBar__f,
   class I), then the owner of the class I is Foo_I_ which is different
   from this.prefix, so clsDenot return cls.denot.copySymDenotation(info
   = this).
2. In NamedType#computeDenot we call prefix.isTightPrefix(d.owner) to
   check if we need to recompute the denotation of this type. The prefix of
   the TypeRef computed by the method in step 1 is Mix___eFoo_I_wBar__f,
   but the owner is Foo_I_, so we end up recomputing the denot, which
   leads to subtyping checks, which leads to a loop.

Since we don't allow class overriding in Dotty I think we can just bypass
the isTightPrefix check when the denotation is a ClassDenotation like
this: https://github.com/smarter/dotty/commits/fix-%23241

Alternatively, we could change the owner of the denot returned by clsDenot
in ClassInfo#typeRef.

—
Reply to this email directly or view it on GitHub
#1000 (comment).

Martin Odersky
EPFL

[smarter]

OK, so why can't we always call symbolicTypeRef in typeRef?

[odersky]

Symbolic refs go stale on reload.

On Tue, Dec 15, 2015 at 12:58 AM, Guillaume Martres <
[email protected]> wrote:

OK, so why can't we always call symbolicTypeRef in typeRef?

—
Reply to this email directly or view it on GitHub
#1000 (comment).

Martin Odersky
EPFL

[smarter]

LGTM, I think I get it, but:

In general we can't prevent the recomputation of the denotation without giving it a fixed sym

I think that ClassInfo#typeRef always returns a type with a SymDenotation valid for the current run, so the only reason for recomputing the denotation is if the prefix is not tight:

          case d: SymDenotation =>
            if (this.isInstanceOf[WithFixedSym]) d.current
            else if (d.validFor.runId == ctx.runId || ctx.stillValid(d))
              if (d.exists && prefix.isTightPrefix(d.owner) || d.isConstructor) d.current
              else recomputeMember(d) // symbol could have been overridden, recompute membership
            else {
              val newd = loadDenot
              if (newd.exists) newd else d.staleSymbolError
            }

So just disabling the isTightPrefix for class denotations should work, but maybe there's some cases I'm missing.

[smarter]

(I said LGTM but the frozen_<:< still need to be added before this is merged).

[odersky]

Is it OK with people if we merge the whole PR? At this stage everything before the PR feels very outdated, and I would not want to base anything on it.

[smarter]

Fine with me.