Skip to content

SI-6646 Fix regression in for desugaring. #1606

Merged
merged 3 commits into from Nov 13, 2012

5 participants

@retronym
The Scala Programming Language member

The early check in the parser of pattern irrefutability,
added in c82ecab, failed to consider InitCaps and
backquoted identifiers.

Review by @paulp

@adriaanm should take a look to judge the severity.

Given that it's a silent killer, I've formed an opinion that is
expressed with the target branch of this PR.

@retronym retronym SI-6646 Fix regression in for desugaring.
The early check in the parser of pattern irrefutability,
added in c82ecab, failed to consider InitCaps and
`backquoted` identifiers.
8b2caf0
@jsuereth
The Scala Programming Language member

I believe, given the spec and that this is regression, it's a blocker.

@paulp
paulp commented Nov 10, 2012

Looks like you're cleaning up after me all over the place. LGTM.

@retronym
The Scala Programming Language member

Review once more by @paulp, please.

@paulp paulp and 1 other commented on an outdated diff Nov 13, 2012
src/reflect/scala/reflect/internal/TreeInfo.scala
@@ -245,6 +245,57 @@ abstract class TreeInfo {
isSelfConstrCall(tree1) || isSuperConstrCall(tree1)
}
+ /**
+ * Does this tree represent an irrefutable pattern match
+ * in the position `for { <tree> <- expr }` based only
+ * on information at the `parser` phase?
+ *
+ * This check is not based on
@paulp
paulp added a note Nov 13, 2012

this check is not based on... WHAT, THEY'RE ENDING THE SEASON THAT WAY?

@retronym
The Scala Programming Language member
retronym added a note Nov 13, 2012

The producers have decided this was too controversial and have rounded out the story.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@paulp paulp commented on the diff Nov 13, 2012
src/reflect/scala/reflect/internal/TreeInfo.scala
+ * is a variable pattern; if the structure matches,
+ * then the remainder is inevitable.
+ *
+ * The following are not variable patterns.
+ *
+ * {{{
+ * foo @ (bar, (`baz`, quux)) // back quoted ident, not at top level
+ * foo @ (bar, Quux) // UpperCase ident, not at top level
+ * }}}
+ *
+ * If the pattern is a simple identifier, it is always
+ * a variable pattern. For example, the following
+ * introduce new bindings:
+ *
+ * {{{
+ * for { X <- xs } yield X
@retronym
The Scala Programming Language member
retronym added a note Nov 13, 2012

I've added the link to the comment. Makes (enough) sense: __ <- is like __ =, case __ => is the odd one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
retronym added some commits Nov 13, 2012
@retronym retronym SI-6646 `ident` or Ident is always new binding.
The previous commit regressed in these cases:

    // no withFilter
    for (X <- List("A single ident is always a pattern")) println(X)
    for (`x` <- List("A single ident is always a pattern")) println(`x`)

At the top level of the LHS of a <-, such identifiers represent
new bindings, not stable identifier patterns.
b9e3ea7
@retronym retronym Update comment. 45cf745
@paulp
paulp commented Nov 13, 2012

Looks good to me after our exciting little side trip to scala-internals.

@adriaanm
The Scala Programming Language member

Excellent. I agree this should be fixed now. Great docs.

@adriaanm adriaanm merged commit ea87ecb into scala:2.10.0-wip Nov 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.