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

Pattern Matching analysis improvements #4919

Merged
merged 4 commits into from Jan 29, 2016

Conversation

Projects
None yet
6 participants
@retronym
Member

retronym commented Jan 27, 2016

No description provided.

@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Jan 27, 2016

@retronym retronym changed the title from Ticket/9630 to Pattern Matching analysis improvements Jan 27, 2016

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 27, 2016

Member

Might be high value enough for a 2.11 backport once we're happy with these changes.

Member

retronym commented Jan 27, 2016

Might be high value enough for a 2.11 backport once we're happy with these changes.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 27, 2016

Member

Review by @adriaanm

Member

retronym commented Jan 27, 2016

Review by @adriaanm

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 27, 2016

Member

Note that this also closes SI-9399 and SI-9411, in addition to the tickets mentioned in the commit titles.

Member

retronym commented Jan 27, 2016

Note that this also closes SI-9399 and SI-9411, in addition to the tickets mentioned in the commit titles.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 27, 2016

Member

Needs more parens:

fail - run/t3126.scala  [compilation failed]% scalac t3126.scala
t3126.scala:7: error: ';' expected but 'match' found.
    try (v: @unchecked) match { case Some(1) => } catch { case _: MatchError => }
                        ^
one error found
Member

adriaanm commented Jan 27, 2016

Needs more parens:

fail - run/t3126.scala  [compilation failed]% scalac t3126.scala
t3126.scala:7: error: ';' expected but 'match' found.
    try (v: @unchecked) match { case Some(1) => } catch { case _: MatchError => }
                        ^
one error found

retronym added some commits Jan 27, 2016

SI-9630 Fix spurious warning related to same-named case accessors
Hash consing of trees within pattern match analysis was broken, and
considered `x1.foo#1` to be the same tree as `x1.foo#2`, even though
the two `foo`-s referred to different symbols.

The hash consing was based on `Tree#correspondsStructure`, but the
predicate in that function cannot veto correspondance, it can only
supplement the default structural comparison.

I've instead created a custom tree comparison method for use in
the pattern matcher that handles the tree shapes that we use.
SI-9398 Treat case classes as one-element ADTs for analysis
Currently, exhaustivity analysis only runs for scrutinees with
a sealed type.

This commit treats any case class as a one-element, sealed type
to enable additional analysis, such as in the new test case.
Fix non-exhaustive match in macro code parsing
Before:

```
⚡ qscala -deprecation
Welcome to Scala 2.12.0-20160126-000825-1e302b76aa (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_66).
Type in expressions for evaluation. Or try :help.

scala> import reflect.macros.blackbox.Context; import language.experimental.macros
import reflect.macros.blackbox.Context
import language.experimental.macros

scala> def impl(c: Context) = {println(c.universe.showRaw(c.parse("val then = 0"))); c.literalUnit}; def m: Unit = macro impl;
<console>:13: warning: method literalUnit in trait ExprUtils is deprecated: Use quasiquotes instead
       def impl(c: Context) = {println(c.universe.showRaw(c.parse("val then = 0"))); c.literalUnit}; def m: Unit = macro impl;
                                                                                       ^
impl: (c: scala.reflect.macros.blackbox.Context)c.Expr[Unit]
defined term macro m: Unit

scala> m
<console>:16: error: exception during macro expansion:
scala.MatchError: pos: source-<macro>,line-1,offset=4 then is now a reserved word; usage as an identifier is deprecated WARNING (of class scala.tools.nsc.reporters.StoreReporter$Info)
	at scala.reflect.macros.contexts.Parsers$class.scala$reflect$macros$contexts$Parsers$class$$$anonfun$1(Parsers.scala:17)
```
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 29, 2016

Member

@adriaanm I've added the parens and a case for Apply nodes.

Member

retronym commented Jan 29, 2016

@adriaanm I've added the parens and a case for Apply nodes.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 29, 2016

Member

LGTM! Great catch of my misuse of correspondsStructure.

Member

adriaanm commented Jan 29, 2016

LGTM! Great catch of my misuse of correspondsStructure.

retronym added a commit that referenced this pull request Jan 29, 2016

Merge pull request #4919 from retronym/ticket/9630
Pattern Matching analysis improvements

@retronym retronym merged commit 6f9c65d into scala:2.12.x Jan 29, 2016

6 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [903] SUCCESS. Took 2 s.
Details
validate-main [1100] SUCCESS. Took 133 min.
Details
validate-publish-core [1087] SUCCESS. Took 17 min.
Details
validate-test [902] SUCCESS. Took 114 min.
Details
@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jan 29, 2016

Member

Very nice. Interesting to note all the tests that needed changing due to this.

Member

dwijnand commented Jan 29, 2016

Very nice. Interesting to note all the tests that needed changing due to this.

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Jan 29, 2016

Contributor

@retronym Cool thanks.

Contributor

paulp commented Jan 29, 2016

@retronym Cool thanks.

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Jan 29, 2016

Contributor

👍 👍 👍
@retronym 🍻 🙇

Contributor

japgolly commented Jan 29, 2016

👍 👍 👍
@retronym 🍻 🙇

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Jan 31, 2016

Contributor

Would it be possible to backport this for the soon-to-be-released 2.11.8? Also, is there anything I can do to help?

Contributor

japgolly commented Jan 31, 2016

Would it be possible to backport this for the soon-to-be-released 2.11.8? Also, is there anything I can do to help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment