Skip to content

Issue with Symbols#mapSymbols leads to various errors when using TreeTypeMap #1812

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
smarter opened this issue Dec 16, 2016 · 4 comments
Closed

Comments

@smarter
Copy link
Member

smarter commented Dec 16, 2016

Every usage of TreeTypeMap is affected, even if run using atGroupEnd like we do in ExtensionMethods, so this is a different issue than #1770:

class FF[R] {
  def compose(): R = ???
}

class Test(x: Int) extends AnyVal {
  def method: Unit = {
    class Bla
    class Foo extends FF[Bla] {
      override def compose() = super[FF].compose()
    }
  }
}

This fails -Ycheck:tailrec with:

java.lang.AssertionError: assertion failed:
found:    Bla
required: Bla'
where:    Bla  is a class in method method
          Bla' is a class in method method$extension

tree = super[FF].compose()

It's pretty hard to understand what's going on here but I think I've narrowed it down to mapSymbols, recall that it works in three steps:

  1. Create new naked symbols for each symbol in original
  2. Set the denotation of each of the copies to the original denotation
  3. Create a new denotation for each of the copy where we set their info using the TreeTypeMap, etc.

The issue happens in 3., to create the new infos we map over the old infos (info = ttmap1.mapType(oinfo)) for each symbol in order, but when we map over the info of one mapped symbol, we may end up looking at another mapped symbol whose denotation is still set to the original denotation, even if we don't end up taking any decision based on that symbol denotation we may end up corrupting some cache (for example, NamedType cache denotations).

More specifically, in the example above:

  1. class Foo is in a method that becomes an extension method, so its symbol ends up being transformed by TreeTypeMap
  2. In TreeTypeMap#withMappedSyms we call mapSymbols with every declaration in Foo, in the list of declarations compose appears before FF$R, this is important.
  3. Inside mapSymbols we get to the step 3 explained above, we map the info of compose which is ()Foo.this.FF$R, this is done successfully but note that at this point the denotation of the new symbol for FF$R is still incorrect, and Foo.this.FF$R is a NamedType which means that we cache its denotation.
  4. Still inside mapSymbols we correctly set the denotation of the new symbol for FF$R but it's too late.

Later in TreeChecker when we look at the type of super[FF].compose() we get the old cached denotation which points to the old symbol for Bla, thus the error.

@smarter
Copy link
Member Author

smarter commented Dec 16, 2016

I wonder if this could be fixed by setting the denotations of mapped symbols lazily in mapSymbols, thought it might be hard to avoid cycles when doing that.

@DarkDimius
Copy link
Contributor

Looking at it. Btw: why area:typer?

@smarter
Copy link
Member Author

smarter commented Dec 19, 2016

Yeah I guess area:transform is more appropriate

@DarkDimius
Copy link
Contributor

In TreeTypeMap#withMappedSyms we call mapSymbols with every declaration in Foo, in the list of declarations compose appears before FF$R, this is important.

Seems to be the source of a bug. As we have forward references, order shouldn't be important and TTM is suposed to handle it.
Preparing a fix.

@DarkDimius DarkDimius self-assigned this Dec 19, 2016
DarkDimius added a commit to dotty-staging/dotty that referenced this issue Dec 19, 2016
…rmation.

Reasoning similar to one in the previous commit also applies to annotations.
DarkDimius added a commit to dotty-staging/dotty that referenced this issue Dec 19, 2016
…rmation.

Reasoning similar to one in the previous commit also applies to annotations.
odersky added a commit that referenced this issue Dec 20, 2016
Fix #1812, Symbols.mapSymbols shouldn't replace denotations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants