newTermNameCached causes grief

[Eugene Burmako]

After I inserted an assertion that guards PackageScope.enter from
entering symbols with the same name twice, it immediately crashed in
my face. I thought there's something in the changes I made to
implement reflection sync, but apparently the reason is much more
interesting.

Here's the simplified logic of PackageScope.lookupEntry(name: Name):
1) At first it searches for an existing symbol in scope.
2) If the symbol lookup succeeds, we return the result.
3) If the symbol lookup fails and then it tries to load a java class
with that name [1].
4) If the class lookup succeeds and then the class is loaded, entered
in scope and assigned a completer [2].
5) Finally, after the lookup is a success, lookupEntry calls itself
expecting to return on step 2 in a recursive call.

The problem is that sometimes the lookup in the recursive call doesn't
succeed, so lookupEntry tries to materialize a symbol again, which
crashes the aforementioned assertion.

How does this happen?
A) On the one hand, when someone does
reflect.runtime.universe.staticModule(name), this name goes through
newTermNameCached and ends up as an argument to lookupEntry.
B) On the other hand, when a class/module pair is entered on step 4,
it calls owner.newModule(term.toTermName) [3], which boils down to
"new TermName(...)".
C) Amazingly enough, the names created in A and in B are referentially
different, even though they are equal (what's more: they are both
instances of TermName_S).
D) Therefore symbol lookup in the recursive call of lookupEntry fails,
because scope lookup uses reference equality to compare names of scope
entries.

Having said all that, I have a single question. Is it intentional that
names A and B are different? If someone can confirm that it's not,
I'll try to fix it.

[1]
https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920327486212f5020/src/reflect/scala/reflect/runtime/SymbolLoaders.scala#L116
[2]
https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920327486212f5020/src/reflect/scala/reflect/runtime/SymbolLoaders.scala#L121
[3]
https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920327486212f5020/src/reflect/scala/reflect/runtime/SymbolLoaders.scala#L63

[Eugene Burmako]

Actually the more I think, the more I get an impression that reference
equality isn't at fault here. After all, when I change
newTermNameCached in staticModule to newTermName, the bug disappears.
Need to go deeper

> --
> You received this message because you are subscribed to the Google Groups "scala-internals" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Eugene Burmako]

Guys, I'm very sorry for wasting your time. I think the problem is in
fact that newTermNameCached isn't synchronized, while newTermName is.
Let me verify, but everything points to this.

On Feb 5, 6:13 pm, Eugene Burmako <xeno...@gmail.com> wrote:
> Actually the more I think, the more I get an impression that reference
> equality isn't at fault here. After all, when I change
> newTermNameCached in staticModule to newTermName, the bug disappears.
> Need to go deeper
>

> >https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920...
> > [2]
> >https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920...
> > [3]
> >https://github.com/scalamacros/kepler/blob/3d318be51f8e8cdec314565920...