Skip to content
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

Run PatternMatcher earlier, in the group before ExplicitOuter #13124

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Jul 21, 2021

fixes #13096, which @dwijnand and I are looking at together

marked as draft for now since this is a naive fix. let's see if anything else regresses, and also consider if the fix can be refined

@SethTisue
Copy link
Member Author

SethTisue commented Jul 21, 2021

I was prompted to even try this fix by this comment in ExplicitOuter.scala:

needs to run after pattern matcher as it can add outer checks and force creation of $outer

I don't know how miniphases work exactly, but in the -Ycheck:all log I saw:

MegaPhase{elimOpaque, tryCatchPatterns, patternMatcher, explicitOuter, explicitSelf, elimByName, ...)

which doesn't seem like "after", or does it? does the order of mini phases within a mega phase matter?

@SethTisue
Copy link
Member Author

SethTisue commented Jul 21, 2021

note that ExplicitOuter already has:

override def runsAfter: Set[String] = Set(PatternMatcher.name, HoistSuperArgs.name)

apparently this isn't having the effect the author (Dmitry in d68c106) intended?

@smarter
Copy link
Member

smarter commented Jul 21, 2021

which doesn't seem like "after", or does it? does the order of mini phases within a mega phase matter?

I don't see anything wrong here, the order of mini-phases in a group does matter so it makes sense to talk about a miniphase running after another (meaning: in a transformXXX(tree: ...) call, tree has already been transformed by every previous mini-phase (and the children of tree have already been transformed by all the mini-phases in the group) I don't know if it'll help but here's me trying to explain this with pictures and hand gestures five years ago: https://youtu.be/wCFbYu7xEJA?t=3701).

@SethTisue
Copy link
Member Author

the CI run shows only a single regression, scala3-compiler-bootstrapped / Test / testOnly dotty.tools.dotc.transform.PatmatExhaustivityTest gives

> java.lang.ClassCastException: dotty.tools.dotc.core.Phases$NoPhase$ cannot be cast to dotty.tools.dotc.core.DenotTransformers$DenotTransformer
> 	at dotty.tools.dotc.transform.ExplicitOuter$.ensureOuterAccessors$$anonfun$1(ExplicitOuter.scala:135)
> 	at scala.collection.immutable.List.foreach(List.scala:333)
> 	at dotty.tools.dotc.transform.ExplicitOuter$.ensureOuterAccessors(ExplicitOuter.scala:135)
> 	at dotty.tools.dotc.transform.PatternMatcher$Translator.outerTest$1(PatternMatcher.scala:736)

@dwijnand
Copy link
Member

The problem is that the change is across siblings:

  class C1() extends Object() {
    private[this] class C2() extends Object() {}
    new C1.this.C2() match { case c @ _:C1.this.C2 => () }
  }

to

  class C1() extends Object() {
    private[this] class C2() extends Object() {
      private[this] val $outer: C1
      final def C1$C2$$$outer: C1 = C2.this.$outer
    }
    matchResult1[Unit]: {
        case val x1: C1.this.C2 = new C1.this.C2()
        if x1.$isInstanceOf[C1.this.C2].&&(x1.1_<outer>.eq(this)) then {
            case val c: C1.this.C2 = x1.$asInstanceOf[C1.this.C2]
            return[matchResult1] { () }
          } else ()
        throw new MatchError(x1)
      }
  }

Pattern matcher changes C1's constructor, which requires a change in the C2 class definition, and those two are siblings.

@SethTisue
Copy link
Member Author

given the sibling thing, it sounds like explicitOuter ought to be using runsAfterGroupsOf instead of just plain runsAfter, then.

miniphase paper:

The runsAfterGroupsOf method returns a set of Miniphases that must strictly precede the fused Mega- phase containing the current Miniphase. In other words, a Miniphase in runsAfterGroupsOf must completely finish transforming the tree before the current Miniphase can run

@SethTisue SethTisue marked this pull request as ready for review July 21, 2021 19:06
@SethTisue
Copy link
Member Author

ready for review! (tests were passing before I squashed)

@dwijnand
Copy link
Member

lol that worked

@dwijnand dwijnand changed the title delay ExplicitOuter until after PatternMatcher Run PatternMatcher earlier, in the group before ExplicitOuter Jul 26, 2021
TryCatchPatterns is similar in spirit, so keep it with its buddy.

Fixes scala#13096

Co-authored-by: Dale Wijnand <[email protected]>
@dwijnand dwijnand marked this pull request as ready for review July 27, 2021 09:31
@dwijnand dwijnand requested a review from smarter July 27, 2021 09:31
@dwijnand dwijnand assigned smarter and unassigned SethTisue Jul 28, 2021
@SethTisue
Copy link
Member Author

This is ready for final review and merge afaict

@smarter smarter merged commit 53252c8 into scala:master Jul 28, 2021
@SethTisue SethTisue deleted the issue-13096 branch July 28, 2021 19:41
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
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.

NoSuchMethodError when pattern matching on inner class (no outer accessor)
4 participants