Fix #1846: add regression test #3810

liufengyun · 2018-01-11T16:25:46Z

Fix #1846: add regression test

This PR does the following:

removes the block syntax in patterns
fix a crash under -Ycheck
check that stable identifier patterns are really stable -- scalac compatible

Note that by removing the block syntax from patterns, following code is still supported:

implicit class Regex(sc: StringContext) {
  def r = new util.matching.Regex(sc.parts.mkString, sc.parts.tail.map(_ => "x"): _*)
}

scala> "123" match { case r"\d+" => true case _ => false }
res34: Boolean = true

scala> "123" match { case r"(\d+)$d" => d.toInt case _ => 0 }
res36: Int = 123

allanrenucci · 2018-01-11T16:44:31Z

Wow! I didn't know we could do that. We should probably spec it and test it thoroughly.

For instance, there are some inconsistencies:

42 match { case { 42 } => 42 } // OK
42 match { case { 42 }.toString => 42 } // OK, should error?
42 match { case { 42.toString } => 42 } // error

allanrenucci · 2018-01-11T17:00:34Z

@liufengyun I see you closed #1774 saying it is a bad idea. Then I think we should make this a neg test and make sure the compiler rejects block in patterns. WDYT?

liufengyun · 2018-01-11T17:02:20Z

@allanrenucci It's actually consistent, I've updated the test.

The following doesn't type-check because the pattern type and scrutinee type are irrelevant:

42 match { case { 42.toString } => 42 }

The following code works:

 "h" match { case { 42.toString } => 42 }

liufengyun · 2018-01-11T17:04:41Z

@allanrenucci Note that blocks are allowed in the syntax specification for Dotty:

SimplePattern     ::=  PatVar                                                   Ident(wildcard)
                    |  Literal                                                  Bind(name, Ident(wildcard))
                    |  ‘(’ [Patterns] ‘)’                                       Parens(pats) Tuple(pats)
                    |  XmlPattern
                    |  SimplePattern1 [TypeArgs] [ArgumentPatterns]
SimplePattern1    ::=  Path
                    |  ‘{’ Block ‘}’
                    |  SimplePattern1 ‘.’ id

I think in #1774, I misunderstood the semantics of blocks in patterns.

allanrenucci · 2018-01-11T17:08:35Z

I understand why this typechecks:

42 match { case { 42.toString } => 42 }

But then I don't understand why this typechecks:

42 match { case { 42 }.toString => 42 }

liufengyun · 2018-01-11T17:23:45Z

The following code shows the pattern:

    val x = 42
    val Y = "h"

    x match { case { "h" }.toString => println(42) }  // ok
    x match { case { "h".toString } => println(42) }  // error
    x match { case Y => println(42) }  // error
    x match { case Y.toString => println(42) }  // ok

But arguably, all of them should be invalid.

allanrenucci · 2018-01-11T17:27:47Z

But arguably, all of them should be invalid.

Can you open an issue?

allanrenucci

One more reason to reject 42 match { case { "h" }.toString => ... }, Ycheck is not happy about it 😄

allanrenucci · 2018-01-11T17:37:27Z

tests/run/i1846.scala

@@ -0,0 +1,22 @@
+object Test {


I think is it better to use assert instead of a check file for simple test cases like this one. Also one more think you could test:

val foo = 42 match { case x @ { 42 } => x } assert(foo == 42)

allanrenucci · 2018-01-12T12:53:18Z

tests/run/i3812.scala

+    x match { case { Y.toInt }      => () } // ok
+    x match { case { Y }.toInt      => () } // ok
+
+    // x match { case Y.toInt          => () } // compiles but rejected by Ycheck


@liufengyun Ycheck fails on this one. Should this be allowed?

Good catch, I think it should be allowed.

Note that case "42".toInt => does not parse. It might be confusing that one is allowed but not the other

The syntax specification requires the prefix has to be a path or block.

SimplePattern ::= PatVar Ident(wildcard) | Literal Bind(name, Ident(wildcard)) | ‘(’ [Patterns] ‘)’ Parens(pats) Tuple(pats) | XmlPattern | SimplePattern1 [TypeArgs] [ArgumentPatterns] SimplePattern1 ::= Path | ‘{’ Block ‘}’ | SimplePattern1 ‘.’ id

allanrenucci · 2018-01-17T14:43:26Z

tests/run/i3812.scala

+    val x = 42
+    val Y = "42"
+
+    x match { case Y.toInt          => () } // ok


This is an error in scalac, I don't think we should support it either

Good point, a check for stable prefix is missing for patterns here.

Now I'm a little unsure about Scalac behavior, as it also rejects the following code:

var X = 42 42 match { case X => () }

It seems to me it can be accepted without any issue.

According to scala spec 2.13, the scalac behavior is correct.

https://www.scala-lang.org/files/archive/spec/2.13/08-pattern-matching.html#stable-identifier-patterns

liufengyun · 2018-01-18T17:04:08Z

@odersky This PR does the following:

removes the block syntax in patterns
fix a crash under -Ycheck
check that stable identifier patterns are really stable -- scalac compatible

Note that by removing the block syntax from patterns, following code is still supported:

implicit class Regex(sc: StringContext) {
  def r = new util.matching.Regex(sc.parts.mkString, sc.parts.tail.map(_ => "x"): _*)
}

scala> "123" match { case r"\d+" => true case _ => false }
res34: Boolean = true

scala> "123" match { case r"(\d+)$d" => d.toInt case _ => 0 }
res36: Int = 123

odersky

Agreed on leaving out blocks in patterns. But the stability checks should be justified and either be eliminated or refactored.

odersky · 2018-01-21T16:18:25Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

@@ -75,6 +75,21 @@ object RefChecks {
    }
  }

+  /** Check that a stable identifier pattern is indeed stable (SLS 8.1.5)
+   */
+  private def checkStableIdentPattern(tree: Tree)(implicit ctx: Context) = {


I believe it would be simpler to put this check in Typer.

Here, we could just add if (ctx.mode.is(Mode.Pattern) && !tree.tpe.isStable) error to the checks for Ident and Select.

But I am also no sure why we should demand stability? What would go wrong if we didn't?

The following check seems too strong to me:

if (ctx.mode.is(Mode.Pattern) && !tree.tpe.isStable) error

As a selection of unapply would fail this check. The check should only happen for Stable Identifier Pattern (SLS 8.1.5). I don't know what's the motivation for the Scala specification. Arguably, a var could be allowed in a pattern, but then it complicates the specification and check.

OK, so let's keep the test for stable idents but move it to Typer.

OK, now it's done.

odersky

LGTM

allanrenucci reviewed Jan 11, 2018

View reviewed changes

allanrenucci mentioned this pull request Jan 11, 2018

Missing relevance check for select in patterns #3812

Closed

liufengyun assigned allanrenucci Jan 12, 2018

allanrenucci reviewed Jan 12, 2018

View reviewed changes

allanrenucci removed their assignment Jan 12, 2018

liufengyun and others added 5 commits January 17, 2018 14:53

Fix scala#1846: add regression test

18d0361

add more tests

96afef1

Add Eq check for Apply tree in adapting patterns

47256d2

Fix scala#3812: Refine tests for pattern match

cff4826

fix retyper in checking Select in patterns

2734958

liufengyun force-pushed the fix-1846 branch from d2d8fab to bf9826e Compare January 17, 2018 13:56

remove block syntax in patterns

0677c77

liufengyun force-pushed the fix-1846 branch from bf9826e to 0677c77 Compare January 17, 2018 13:58

allanrenucci reviewed Jan 17, 2018

View reviewed changes

check qualifier is stable in pattern select

2fe7940

liufengyun force-pushed the fix-1846 branch from e0e6284 to 2fe7940 Compare January 18, 2018 13:22

liufengyun added 2 commits January 18, 2018 14:30

remove noise in test code

798b0d3

revert the change to typer

77ac75c

liufengyun assigned odersky Jan 18, 2018

odersky requested changes Jan 21, 2018

View reviewed changes

odersky assigned liufengyun and unassigned odersky Jan 21, 2018

liufengyun assigned odersky and unassigned liufengyun Jan 22, 2018

odersky assigned liufengyun and unassigned odersky Jan 22, 2018

address review: move check to typer

4630203

liufengyun assigned odersky and unassigned liufengyun Jan 22, 2018

odersky approved these changes Jan 27, 2018

View reviewed changes

odersky merged commit 46956b0 into scala:master Jan 27, 2018

allanrenucci deleted the fix-1846 branch January 27, 2018 17:32

Fix #1846: add regression test #3810

Fix #1846: add regression test #3810

Uh oh!

Conversation

liufengyun commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci commented Jan 11, 2018

Uh oh!

allanrenucci commented Jan 11, 2018

Uh oh!

liufengyun commented Jan 11, 2018

Uh oh!

liufengyun commented Jan 11, 2018

Uh oh!

allanrenucci commented Jan 11, 2018

Uh oh!

liufengyun commented Jan 11, 2018

Uh oh!

allanrenucci commented Jan 11, 2018

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengyun Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengyun commented Jan 18, 2018

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liufengyun commented Jan 11, 2018 •

edited

Loading

liufengyun Jan 18, 2018 •

edited

Loading