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

Transition from pattern match based on extractor with a typed parameter in unapply() skips next case #2337

Closed
scabug opened this issue Sep 8, 2009 · 11 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Sep 8, 2009

Not sure if this is a known limitation, an existing bug, or a new bug in 2.8.

I have a value 'element' of runtime type:

class ScTypeParameterImpl extends 
  (trait ScTypeParam extends 
    (trait ScPolymorphicElement extends ScNamedElement)))

trait ScNamedElement extends ... {
   // abstract defs
   // implementations
}

It was fed into the pattern match. I expected it to match x: !ScNamedElement. Instead, it matched the default case. I added an explicit case for 'x: !ScTypeParam' as a workaround.

    val name = element match {
      case RealPsiClass(c) => c.getQualifiedName
      case x: PsiMethod => PsiFormatUtil.formatMethod(x, PsiSubstitutor.EMPTY,
        PsiFormatUtilBase.SHOW_NAME | PsiFormatUtilBase.SHOW_PARAMETERS,
        PsiFormatUtilBase.SHOW_TYPE) + " of " + getDescriptiveName(x.getContainingClass)
      case x: PsiVariable => x.getName
      case x: PsiFile => x.getName

      case x: ScNamedElement => x.getName // todo This line should match, but doesn't.
      case x: ScTypeParam => (x: ScNamedElement).getName // todo Should even compile! Previous pattern should match!

      case _ => element.getText
    }
    Option(name) getOrElse "anonymous"
  }

Taking a closer look with the debugger, it appears, at least from a Java perspective, that the class literal !ScNamedElement.class refers to the class containing the method definitions from trait !ScNamedElement, rather than its abstract interface.

Debugger:

element.getClass() = class org.jetbrains.plugins.scala.lang.psi.impl.statements.params.ScTypeParamImpl

element.getClass().getInterfaces()r1.getInterfaces()r1.getInterfaces()r2 = {java.lang.Class@10972}"interface org.jetbrains.plugins.scala.lang.psi.api.toplevel.ScNamedElement"

ScNamedElement.class = {java.lang.Class@10978}"class org.jetbrains.plugins.scala.lang.psi.api.toplevel.ScNamedElement$$class"

ScNamedElement.class.isAssignableFrom(element.getClass()) == false

Class.forName("org.jetbrains.plugins.scala.lang.psi.api.toplevel.ScNamedElement").isAssignableFrom(element.getClass()) = true

My brief attempt to isolate this to a small testcase were unsuccessful. If this isn't enough to pinpoint the problem, and it really is a bug, I'll spend some more time at this.

Tested against r18650

thanks,

-jason

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 8, 2009

Imported From: https://issues.scala-lang.org/browse/SI-2337?orig=1
Reporter: @retronym
Attachments:

  • 2337.check (created on Sep 22, 2009 8:25:25 AM UTC, 12 bytes)
  • 2337.scala (created on Sep 22, 2009 8:25:07 AM UTC, 296 bytes)
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 15, 2009

@cunei said:
Should go to pattern matcher maintainers (MacIver? Paulp?)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 15, 2009

@cunei said:
We cannot decode the source of this issue; can you please provide a simplified and self-contained test case?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 15, 2009

@paulp said:
It's paulp, but I also would really appreciate a self-contained test case.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 21, 2009

@retronym said:
I just found #1697, which sounds similar.

This code:

  def matchBroken(element: PsiElement) = element match {
    case RealPsiClass(c) => "matched: RealPsiClass(c)"
    case x: ScNamedElement => "matched: x: ScNamedElement"
    // This is matched if element is a ScTypeParam, should have been the previous case.
    case x if x.isInstanceOf[ScNamedElement] => "matched: x if x.isInstanceOf[ScNamedElement]"
    case _ => "matched _"
  }

compiled against a r18583, then decompiled with JAD, gives:

  public String matchBroken(PsiElement element) {
    PsiClass temp2;
    if (element instanceof PsiClass) {
      if (RealPsiClass$$.MODULE$$.unapply(temp2 = (PsiClass) element).isEmpty()) {
        if (gd1$$1(temp2)) {
          return "matched: x if x.isInstanceOf[ScNamedElement]";
        } else {
          return "matched _";
        }
      } else {
        return "matched: RealPsiClass(c)";
      }
    }
    else if (element instanceof ScNamedElement) {
      return "matched: x: ScNamedElement";
    } else if (gd1$$1(element)) {
      return "matched: x if x.isInstanceOf[ScNamedElement]";
    }
    return "matched _";
  }

Removing the first case, which uses an extractor object, fixes the problem.

When I tried to make this a stand-alone example, replacing all referenced types with scala traits, the problem disappears. Maybe having a java super-interface has some impact.

Still digging :(

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 22, 2009

@retronym said:
Okay, I've boiled it down to a simple test case, see attachments.

Here is the tree generated by the pattern match:

{
  var temp1: this.T1 = ({
    final class $$anon extends java.lang.Object with this.T2 with this.U1 {
      def this($$outer: anonymous class $$anon): anonymous class $$anon = {
        $$anon.super.this();
        ()
      };
      <synthetic> <paramaccessor> private[this] val $$outer: anonymous class $$anon = _;
      <synthetic> <stable> def main$$$$anon$$$$anon$$$$$$outer(): anonymous class $$anon = $$anon.this.$$outer
    };
    new anonymous class $$anon($$anon.this)
  }: this.T1);
  if (temp1.isInstanceOf[this.T2]())
    {
      {
        {
          {
            if ($$anon.this.Extractor().unapply(temp1.asInstanceOf[this.T2]()).isEmpty().unary_!())
              {
                {
                  {
                    "Extractor(_)"
                  }
                }
              }
            else
              {
                "matched _"
              }
          }
        }
      }
    }
  else
    if (temp1.isInstanceOf[this.T1 with this.U1]())
      {
        {
          {
            {
              "matched: U1"
            }
          }
        }
      }
    else
      {
        "matched _"
      }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented May 25, 2010

@paulp said:
See #3479 for another failing example.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 13, 2010

Andrey (andrey) said:
Another failing example ([http://stackoverflow.com/questions/3915866/number-number-matches-float-int-but-does-not-match-int-float])

import junit.framework._
import Assert._

class BugTest extends TestCase {

  def compare(first: Any, second: Any): Int = {
      (first, second) match {
        case (k: Int, o: Int) => k compare o
        //why the next case matches (Float, Int) but does not match (Int, Float) ???
        case (k: Number, o: Number) => k.doubleValue() compare o.doubleValue()
        case _ => throw new Exception("Unsupported compare " + first + "; " + second)
    }
  }

  def testCompare() {
    assertEquals("Both Int", -1, compare(0, 1))
    assertEquals("Both Float", 1, compare(1.0, 0.0))
    assertEquals("Float then Int", 0, compare(10.0, 10))
    assertEquals("Int then Float", 0, compare(10, 10.0))//this fails with an exception
  }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Nov 6, 2010

Alex Black (alexblack) said:
I believe this is another failing example:

object App {
object Extractor {
def unapply(s: List[String]): Option[Boolean] = {
if ( s == List("1", "2", "3") )
Some(true)
else
None
}
}

def matcher1: PartialFunction[List[String], String] = {
case Extractor(_) => "GOOD"
case x @ List("foo") => "FOO"
}

def matcher2: PartialFunction[List[String], String] = {
case x @ List("foo") => "FOO"
case Extractor(_) => "GOOD"
}

def main(args: Array[String]): Unit = {
val x = List("1", "2", "3")
println(matcher1.isDefinedAt(x))
println(matcher2.isDefinedAt(x))
}
}

I expected the output to be:

true
true

But, the output is: (on Scala 2.8.0 final)

true
false

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Nov 8, 2011

Commit Message Bot (anonymous) said:
(moors in r25966) smarter bridges to unapplies

wraps the call to a bridged synthetic unapply(Seq) in a defensive if-test:

if (x.isInstanceOf[expectedType]) real.unapply(x.asInstanceOf[expectedType]) else None

NOTE: the test is WRONG, but it has to be due to #1697/#2337 -- once those are fixed, this one should generate the expected output

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented May 4, 2012

@paulp said:
virtpatmat saves day, test in 8bc8b83f0b .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.