8000 SI-3772 Fix detection of term-owned companions by retronym · Pull Request #5550 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

SI-3772 Fix detection of term-owned companions #5550

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from :ticket/3772
Dec 12, 2016
Merged

SI-3772 Fix detection of term-owned companions #5550

merged 2 commits into from
Dec 12, 2016

Conversation

retronym
Copy link
Member
@retronym retronym commented Nov 21, 2016 •
edited by SethTisue
Loading

Companion detection consults the scopes of enclosing Contexts during
typechecking to avoid the cycles that would ensue if we had to look
at into the info of enclosing class symbols. For example, this used
to typecheck:

object CC { val outer = 42 }
if ("".isEmpty) {
  case class CC(c: Int)
  CC.outer
}

This logic was not suitably hardened to find companions in exactly
the same nesting level.

After fixing this problem, a similar problem in Namer::inCurrentScope
could be solved to be more selective about synthesizing a companion
object. In particular, if a manually defined companion trails after
the case class, don't create an addiotional synthetic comanpanion object.

JIRA: https://issues.scala-lang.org/browse/SI-3772

@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Nov 21, 2016
@retronym retronym self-assigned this Nov 21, 2016
@retronym retronym added the WIP label Nov 21, 2016
@retronym
Copy link
Member Author

@som-snytt Could you think of any additional test cases for this?

@som-snytt
Copy link
Contributor

This progresses to a VerifyError. (https://issues.scala-lang.org/browse/SI-8733)

I was thinking of those odd constructor contexts. Maybe there's a better corner case; I've forgotten the context.

scala> class X(x: Int) { def f = x }
defined class X

scala> class Y extends X({ case class C(c: Int) ; object C { val x = 42 } ; C(17).c + C.x })
defined class Y

scala> new Y().f
java.lang.VerifyError: Bad type on operand stack
Exception Details:
  Location:
    Y.<init>()V @11: invokespecial
  Reason:
    Type uninitializedThis (current frame, stack[1]) is not assignable to 'Y'
  Current Frame:
    bci: @11
    flags: { flagThisUninit }
    locals: { uninitializedThis, 'scala/runtime/LazyRef' }
    stack: { uninitializedThis, uninitializedThis, 'scala/runtime/LazyRef' }
  Bytecode:
    0x0000000: 2abb 001f 59b7 0038 4c2a 2bb7 003a 1011
    0x0000010: b600 3eb6 0042 2a2b b700 3ab6 0045 60b7
    0x0000020: 0048 b1

  ... 31 elided

scala> class C extends { val x = { case class C(c: Int) ; object C { val x = 42 } ; C(17).c + C.x } } with AnyRef { def f = x }
defined class C

scala> new C().f
java.lang.VerifyError: Bad type on operand stack
Exception Details:
  Location:
    C.<init>()V @10: invokespecial
  Reason:
    Type uninitializedThis (current frame, stack[0]) is not assignable to 'C'
  Current Frame:
    bci: @10
    flags: { flagThisUninit }
    locals: { uninitializedThis, top, 'scala/runtime/LazyRef' }
    stack: { uninitializedThis, 'scala/runtime/LazyRef' }
  Bytecode:
    0x0000000: bb00 2259 b700 394d 2a2c b700 3b10 11b6
    0x0000010: 003f b600 422a 2cb7 003b b600 4360 3c2a
    0x0000020: 1bb5 0018 2ab7 0044 b1

  ... 31 elided

@som-snytt
Copy link
Contributor
som-snytt commented Nov 21, 2016 •
edited
Loading

There was a test for your other case that fails now:

test/files/pos/t8002-nested-scope.scala

@retronym retronym force-pushed the ticket/3772 branch 3 times, most recently from f1a4d59 to 83d1197 Compare November 21, 2016 23:06
@som-snytt
Copy link
Contributor

What if partest --update-check also shuttled tests between pos and neg.

lrytz
lrytz reviewed Nov 25, 2016
View reviewed changes
src/reflect/scala/reflect/internal/Scopes.scala
case entry if entry.sym == original =>
entry.owner.lookupAllEntries(original.name.companionName)
.filter(_.owner eq entry.owner)
.filter(x => original.isTerm || x.sym.hasModuleFlag).toList match {
Copy link
Member

Choose a reason for hiding this comment

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

A comment on this filter would help - i guess original is either a class symbol or a module symbol? I see it was copied from the original code, but it's not very clear. Also, the original code had a check for isCoDefinedWith - is this no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've re-added the isCodefinedWith check, moved some code around, and added a comment for the hasModuleFlag check.

@retronym
SI-3772 Fix detection of term-owned companions
37037fe 
Companion detection consults the scopes of enclosing Contexts during
typechecking to avoid the cycles that would ensue if we had to look
at into the info of enclosing class symbols. For example, this used
to typecheck:

    object CC { val outer = 42 }
    if ("".isEmpty) {
      case class CC(c: Int)
      CC.outer
    }

This logic was not suitably hardened to find companions in exactly
the same nesting level.

After fixing this problem, a similar problem in `Namer::inCurrentScope`
could be solved to be more selective about synthesizing a companion
object. In particular, if a manually defined companion trails after
the case class, don't create an addiotional synthetic comanpanion object.
lrytz
lrytz reviewed Nov 29, 2016
View reviewed changes
src/reflect/scala/reflect/internal/Scopes.scala
final def lookupCompanion(original: Symbol): Symbol = {
lookupSymbolEntry(original) match {
case null =>
case entry if entry.sym == original =>
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the implementation of lookupSymbolEntry, the guard here seems redundant.

src/reflect/scala/reflect/internal/Scopes.scala
// 1) Must be owned by the same Scope, to ensure that in
// `{ class C; { ...; object C } }`, the class is not seen as a comaniopn of the object.
// 2) Must be in exactly the same scope, so that `{ class C; def C }` are not companions
// TODO Exclude type aliases as companions of terms?
Copy link
Member

Choose a reason for hiding this comment

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

yes, we should. sym.companionClass also does since https://github.com/scala/scala/pull/5482/commits

src/compiler/scala/tools/nsc/typechecker/Contexts.scala
final def lookupCompanionOf(original: Symbol): Symbol = {
lookupScopeEntry(original) match {
case null => NoSymbol
case entry => entry.owner.lookupCompanion(original).filter(_.isCoDefinedWith(original))
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to put the filter here, instead of inside lookupCompanion?

@retronym
Copy link
Member Author

Pushed a commit with your suggestions. We could go a bit further and change Symbol#companion{Class,Module} to internally delegate to flatOwnerInfo.decls.lookupCompanion(this), but I wouldn't make that change without benchmarking.

@lrytz
Copy link
Member
lrytz commented Nov 29, 2016

LGTM, thanks!

@retronym retronym removed the WIP label Nov 29, 2016
@lrytz lrytz merged commit d34e44e into scala:2.12.x Dec 12, 2016
@retronym retronym mentioned this pull request Dec 13, 2016
Closed
@retronym retronym added the release-notes worth highlighting in next release notes label Dec 13, 2016
retronym added a commit to retronym/scala that referenced this pull request Dec 13, 2016
@retronym
Modules w/ serializable type alias "companions" are not serializable
0e770a3 
The behaviour changed in scala#5550, this commit adapts to the change so
that we'll be binary compatible after boostrapping.

MiMa alerted us to a change in the parentage of two objects in the
forkjoin package object.

In Scala 2.12.0/1, they implemented `scala.Serializable`. Recently,
this (synthetically added) parent was absent. This appears to be
due to a bug fix in `companionSymbolOf`, which no longer treats
objects and same-named type aliases to be companions.

This commit manually adds the formerly-synthetic parents to these
objects, and documents the change in compiler behaviour with a test.

Fixes scala/scala-dev#290
retronym added a commit to retronym/scala that referenced this pull request Dec 13, 2016
@retronym
Modules w. serializable type alias "companions" are not serializable
64a0ec4 
The behaviour changed in scala#5550, this commit adapts to the change so
that we'll be binary compatible after boostrapping.

MiMa alerted us to a change in the parentage of two objects in the
forkjoin package object.

In Scala 2.12.0/1, they implemented `scala.Serializable`. Recently,
this (synthetically added) parent was absent. This appears to be
due to a bug fix in `companionSymbolOf`, which no longer treats
objects and same-named type aliases to be companions.

This commit manually adds the formerly-synthetic parents to these
objects, and documents the change in compiler behaviour with a test.

Fixes scala/scala-dev#290
@SethTisue SethTisue mentioned this pull request Jan 13, 2017
Closed
retronym added a commit to retronym/scala that referenced this pull request Jan 19, 2017
@retronym
SI-10154 Fix implicit search regression for term-owned objects
3d38128 
A recent change  to fix lookup of companion implicits of term-owned
classes (scala#5550) caused a regression in the enclosed test case. The
previous approach of calling `Symbol#companionModule` was replaced with
`Namers#companionSymbolOf`. However, this fails to determine that
a method-owned object is the companion of its own module class, as
the module class itself is not entered into the scope of the method.

This commit uses `Symbol#companionSymbol` when the original symbol
is a module class. All term-owned module class symbols will from
the current run, and are represented with `ModuleClassSymbol`,
which ...
retronym added a commit to retronym/scala that referenced this pull request Jan 19, 2017
@retronym
SI-10154 Fix implicit search regression for term-owned objects
1d41aef 
A recent change  to fix lookup of companion implicits of term-owned
classes (scala#5550) caused a regression in the enclosed test case. The
previous approach of calling `Scope#lookup(companionName)` was
replaced by a lookup of the scope entry of the original name followed
by a narrower search lookup for the companion name, to ensure
that it was a true companion, and not just a same-named module
from defined at a different nested scope.

However, module class symbols are not themselves entered into
scopes, so the first part of the new scheme fails. We need to
add a special case modules here.

I've chosen to just call `.sourceModule` on module classes.
For module classes in the current run (all term owned symbols
will fall into this category), this amounts to using the value
of the field `ModuleClassSymbol#module`.
@retronym retronym mentioned this pull request Jan 19, 2017
Merged
@SethTisue SethTisue mentioned this pull request Feb 18, 2017
Merged
This was referenced Apr 7, 2017
Closed
Closed
Closed
@scabug scabug mentioned this pull request Apr 24, 2017
Closed
This was referenced Nov 26, 2021
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz left review comments

Assignees

@retronym retronym

Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.12.2
Development

Successfully merging this pull request may close these issues.
4 participants
@retronym @som-snytt @lrytz @scala-jenkins
0