Skip to content

Fix #10573: Devirtualize member selection when matching #10576

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

Merged
merged 1 commit into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions compiler/src/scala/quoted/runtime/impl/Matcher.scala
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ object Matcher {
scrutinee =?= expr2

/* Match selection */
case (ref: Ref, Select(qual2, _)) if symbolMatch(scrutinee.symbol, pattern.symbol) =>
case (ref: Ref, Select(qual2, _)) if symbolMatch(scrutinee, pattern) =>
ref match
case Select(qual1, _) => qual1 =?= qual2
case ref: Ident =>
Expand All @@ -240,7 +240,7 @@ object Matcher {
case _ => matched

/* Match reference */
case (_: Ref, _: Ident) if symbolMatch(scrutinee.symbol, pattern.symbol) =>
case (_: Ref, _: Ident) if symbolMatch(scrutinee, pattern) =>
matched

/* Match application */
Expand Down Expand Up @@ -348,10 +348,19 @@ object Matcher {
* - The scrutinee has is in the environment and they are equivalent
* - The scrutinee overrides the symbol of the pattern
*/
private def symbolMatch(scrutinee: Symbol, pattern: Symbol)(using Env): Boolean =
scrutinee == pattern
|| summon[Env].get(scrutinee).contains(pattern)
|| scrutinee.allOverriddenSymbols.contains(pattern)
private def symbolMatch(scrutineeTree: Tree, patternTree: Tree)(using Env): Boolean =
val scrutinee = scrutineeTree.symbol
val devirtualizedScrutinee = scrutineeTree match
case Select(qual, _) =>
val sym = scrutinee.overridingSymbol(qual.tpe.typeSymbol)
if sym.exists then sym
else scrutinee
case _ => scrutinee
val pattern = patternTree.symbol

devirtualizedScrutinee == pattern
|| summon[Env].get(devirtualizedScrutinee).contains(pattern)
|| devirtualizedScrutinee.allOverriddenSymbols.contains(pattern)
Copy link
Contributor

@liufengyun liufengyun Dec 1, 2020

Choose a reason for hiding this comment

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

The documentation of the method needs to be updated. The following code might be faster:

scrutinee.overridingSymbol(pattern.owner) == pattern
|| pattern.overridingSymbol(scrutinee.owner) == scrutinee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try that. The current performance is unaccepable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

overridingSymbol worked, but not there.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the check is whether the two symbols override each other, can we simply replace the body of the method with the following:

scrutinee.overridingSymbol(pattern.owner) == pattern
|| pattern.overridingSymbol(scrutinee.owner) == scrutinee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That version had some false positives involving modules.


private object ClosedPatternTerm {
/** Matches a term that does not contain free variables defined in the pattern (i.e. not defined in `Env`) */
Expand Down
3 changes: 3 additions & 0 deletions compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2349,6 +2349,9 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler
def paramSymss: List[List[Symbol]] = self.denot.paramSymss
def primaryConstructor: Symbol = self.denot.primaryConstructor
def allOverriddenSymbols: Iterator[Symbol] = self.denot.allOverriddenSymbols
def overridingSymbol(ofclazz: Symbol): Symbol =
if ofclazz.isClass then self.denot.overridingSymbol(ofclazz.asClass)
else dotc.core.Symbols.NoSymbol

def caseFields: List[Symbol] =
if !self.isClass then Nil
Expand Down
6 changes: 6 additions & 0 deletions library/src/scala/quoted/Quotes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3624,6 +3624,12 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
/** Returns all symbols overridden by this symbol. */
def allOverriddenSymbols: Iterator[Symbol]

/** The symbol overriding this symbol in given subclass `ofclazz`.
*
* @param ofclazz is a subclass of this symbol's owner
*/
def overridingSymbol(ofclazz: Symbol): Symbol

/** The primary constructor of a class or trait, `noSymbol` if not applicable. */
def primaryConstructor: Symbol

Expand Down
16 changes: 16 additions & 0 deletions tests/pos-macros/i10573/Macro_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import scala.quoted._


trait A { def foo: Int }
class B extends A { def foo: Int = 1 }

inline def test(): Unit = ${ testExpr() }

def testExpr()(using Quotes): Expr[Unit] = {
val e0: Expr[A] = '{ new B }
val e1: Expr[Int] = '{ $e0.foo }
e1 match
case '{ ($x: B).foo } => '{ val b: B = $x; () }
case _ => quotes.reflect.report.throwError("did not match")
}

2 changes: 2 additions & 0 deletions tests/pos-macros/i10573/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

def Test = test()