Skip to content

Conversation

@retronym
Copy link
Member

The enclosed test case stopped working in 2.11.5 on the back of
#4040.

The key change was that we ran all post-typer info transformers
on each run of the compiler, rather than trying to reuse the results
of the previous run.

In that patch, I noticed another place in specialization that
aggressively entered specialized members into the owning scope,
rather than relying on transformInfo to place the new members
in the scope of the newly created element of the info history.
I made that change after noticing that this code could actually
mutated scopes of specializaed types at the parser phase, which
led to fairly obscure failures.

This bug is another one of these obscure failures, and has the
same root cause. We effectively "double specialiaze" Function0,
which trips an assertion when method apply$mcI$sp is found
twice in a scope.

I have found another spot that was directly manipulating the scope,
and removed the offending code.

@retronym
Copy link
Member Author

Review by @VladUreche @lrytz

@som-snytt
Copy link
Contributor

Before I left the office, I started looking at the previous PR to get oriented, but I guess that in my time zone, I'll have to get up pretty early in the morning to make a contribution.

@scala-jenkins scala-jenkins added this to the 2.11.6 milestone Jan 15, 2015
@retronym retronym force-pushed the ticket/9089 branch 2 times, most recently from 60851c7 to 87742b2 Compare January 15, 2015 03:06
@VladUreche
Copy link
Contributor

LGTM!

Seems adding the member to the scope is already done by adding it to decls1 (https://github.com/retronym/scala/blob/ticket/9089/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala#L766-L784). The side-effectful enter was added with Alex's PR for abstract override: axel22@9733f56#diff-aaabb8195ea0aa7c456b22d6fc6079d7R873 and it wasn't necessary even then. Shame on me for not having caught that when I reviewed #1173!

The enclosed test case stopped working in 2.11.5 on the back of
scala#4040.

The key change was that we ran all post-typer info transformers
on each run of the compiler, rather than trying to reuse the results
of the previous run.

In that patch, I noticed one place [1] in specialization that
aggressively entered specialized members into the owning scope,
rather than relying on `transformInfo` to place the new members
in the scope of the newly created element of the info history.
I made that change after noticing that this code could actually
mutated scopes of specializaed types at the parser phase, which
led to fairly obscure failures.

This bug is another one of these obscure failures, and has the
same root cause. We effectively "double specialiaze" Function0,
which trips an assertion when `method apply$mcI$sp` is found
twice in a scope.

I have found another spot that was directly manipulating the scope,
and removed the offending code.

[1] scala#4040 (comment)
@lrytz
Copy link
Member

lrytz commented Jan 15, 2015

LGTM, thanks for the links Vlad.

VladUreche pushed a commit that referenced this pull request Jan 16, 2015
SI-9089 Another REPL/FSC + specialization bug fix
@VladUreche VladUreche merged commit d395e52 into scala:2.11.x Jan 16, 2015
@VladUreche
Copy link
Contributor

Thanks @retronym and @lrytz!

@som-snytt
Copy link
Contributor

Ditto the thanks. We don't take you for granted.

@hjacobs
Copy link

hjacobs commented Jan 20, 2015

Is this pull request also fixing the following compiler error (Java 1.8.0_25 and Scala 2.11.5)?

val m = Map(1->2, 3->4)
for((key,value) <- m) yield key

crashes the compiler:
java.lang.AssertionError: assertion failed: List(value _1$mcI$sp, value _1$mcI$sp, value _1$mcI$sp, value _1$mcI$sp)
at scala.reflect.internal.Symbols$Symbol.suchThat(Symbols.scala:1916)

@gkossakowski
Copy link
Contributor

Hi @hjacobs. Yes it does fix this problem.

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.

7 participants