Skip to content

Add missing phase travel when searching a companion in the backend#5976

Merged
retronym merged 1 commit intoscala:2.12.xfrom
lrytz:sd402
Jul 7, 2017
Merged

Add missing phase travel when searching a companion in the backend#5976
retronym merged 1 commit intoscala:2.12.xfrom
lrytz:sd402

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Jul 6, 2017

When looking for the companion of a nested class A$C in the backend, a
pahse travel is necessary to avoid finding the package-owned symbol for
A$C.class (created by SymbolLoaders) instead of the symbol C owned by A.

BTypesFromSymbols contains many such phase travels already, this fixes
a missing case.

Fixes scala/scala-dev#402

When looking for the companion of a nested class A$C in the backend, a
pahse travel is necessary to avoid finding the package-owned symbol for
A$C.class (created by SymbolLoaders) instead of the symbol C owned by A.

BTypesFromSymbols contains many such phase travels already, this fixes
a missing case.

Fixes scala/scala-dev#402
@sjrd
Copy link
Member

sjrd commented Jul 6, 2017

Huh. Is it also what we should be doing in Scala.js instead of our companionModuleClass hack?

@lrytz
Copy link
Member Author

lrytz commented Jul 6, 2017

That sounds plausible, give it a try!

// is compiled from source, this ensures that `companionModule` doesn't return the `A$B`
// symbol created for the `A$B.class` file on the classpath, which might be different.
val companion = exitingPickler(classSym.companionModule)
val staticMethods = companion.info.decls.iterator.filter(m => !m.isConstructor && keepMember(m))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a quick look to see why the flatten info transformer doesn't move the module symbol into the package scope. This !sym.isStaticModule check seems to be related.

Copy link
Member

@retronym retronym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this fix follows the prevailing pattern in the backend.

@sjrd
Copy link
Member

sjrd commented Jul 6, 2017

That sounds plausible, give it a try!

Thanks, I will. scala-js/scala-js#3042

@retronym
Copy link
Member

retronym commented Jul 6, 2017

Logging the idea to fix this in flatten as scala/scala-dev#403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants