Fix #3015: exhaustivity check on top of native apply #3074

liufengyun · 2017-09-06T00:59:01Z

Fix #3015: exhaustivity check on top of native apply

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Added" instead of "Add")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

AppliedType will be used for all type applications, higher-kinded or not. That is, eventually, it will replace HKApply and also the encodings of type applications as refined types. This commit introduces AppliedType and adapts baseType computations to take it into account. AppliedType is not yet constructed anywhere, however.

So far, everything up to parents is adapted to new scheme. Parents is half done; needs more work once we change layour of ClassInfo.

Simplifies usage and removes some obscure code in Types

Systematically introduce handlers for AppliedTypes where there was a handler for HKApply.

Also, change asSeenFrom to use it for arguments

Avoid printing addresses and reduce junk.

We had a stackoverfloa in asSeenFrom even under the old scheme, since there was a bad change involving checkRealizableBounds.

Otherwise we always need to a clean compile to update things.

The previous type parameter representation in terms of type members achieved bounds propagation by waiting until a type member was selected and then taking original bounds and refinements together as its info. This no longer works with explicit applications. Instead, we need to propagate bounds into wildcard arguments explicitly, when a type application is created. Also, fix argument computation in asSeenFrom

Need to avoid variance 0, as it leads to leaking ranges.

Strange as it sounds, a TypeBounds can be a subtype of a single type.

Needed to compile i0239.scala correctly.

Test case was extended and several fixes were necessary to make it pass.

Re-fixes scala#938

- Several fixes for TypeArgRef. - argForParam needs to follow owner chains like toPrefix does (see t119.scala)

Needed to make i2250.scala compile

Can't rely any ore that refinements are stripped away in normalizeClassRef

Without the tweak, neg/i1181c.scala gives the non-sensical message `type mismatch: found: Foo[Int, Int], expected: Any`.

scala#3054 has a better approach.

liufengyun · 2017-09-07T15:27:21Z

test performance please

dottybot · 2017-09-07T15:27:30Z

performance test scheduled: 1 job(s) in queue.

dottybot · 2017-09-07T16:07:03Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3074 to see the changes.

Benchmarks is based on merge(s) with master

liufengyun · 2017-09-08T08:39:54Z

The performance graph shows that the new exhaustivity check performs the same on normal patmat tests as before, but degrades significantly on the abnormal test case V: from 2s to 3.5s.

odersky · 2017-09-08T11:28:35Z

We see now a much bigger performance gain than with applied types alone on dotc. It's about 10% faster as opposed to maybe 2 or 3% before. Interesting, given that exhaustivity checks are now slower than before. My guess is that the reporting of the spurious exhaustivity warnings in dotc itself (there were about 300 of them) impacted overall performance. Anyway, getting 10% on dotc and 6-7 % on Vector or stdlib is very encouraging!

liufengyun · 2017-09-08T11:32:13Z

@odersky The printing of exhaustivity warnings involves a lot of IO, which has a bad impact on performance. You can also test performance by disable the exhautivity check in the pattern matcher, and see how it improves the numbers.

odersky

I think this is a definite improvement. Except for my minor remarks, LGTM

odersky · 2017-09-08T11:31:21Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

    case _ =>
      debug.println(s"unknown pattern: $pat")
      Empty
  }

+  /* Erase a type binding according to erasure semantics in pattern matching */
+  def erase(tp: Type): Type = tp match {
+    case tp@AppliedType(tycon, args) => erase(tp.superType)


Useless erase, this is thrown away

odersky · 2017-09-08T11:31:59Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

+  def erase(tp: Type): Type = tp match {
+    case tp@AppliedType(tycon, args) => erase(tp.superType)
+      if (tycon.isRef(defn.ArrayClass)) tp.derivedAppliedType(tycon, args.map(erase))
+      else tp.derivedAppliedType(tycon, args.map(t => WildcardType(TypeBounds.empty)))


I think you can just use the WildcardType object here.

odersky · 2017-09-08T11:39:55Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

+      def apply(t: Type): Type = t match {
+
+        case tp: TypeRef if tp.underlying.isInstanceOf[TypeBounds] =>
+          // See tests/patmat/gadt.scala  tests/patmat/exhausting.scala


Add: if tp.symbol.is(TypeParam) &&

smarter · 2017-09-08T12:33:43Z

The performance graph shows that the new exhaustivity check performs the same on normal patmat tests as before, but degrades significantly on the abnormal test case V: from 2s to 3.5s.

Do you know why https://github.com/liufengyun/bench/blob/master/tests/exhaustivity-V.scala performs worse now?

liufengyun · 2017-09-08T13:21:48Z

@smarter I think it's because the type inference approach is more expensive than the previous hack for generics.

smarter · 2017-09-08T13:25:52Z

But there is nothing to infer in https://github.com/liufengyun/bench/blob/master/tests/exhaustivity-V.scala, right? Maybe we could avoid the expensive use of type inference in some cases. (Of course this shouldn't be required to get this PR in, but maybe something to investigate later)

liufengyun · 2017-09-08T13:32:20Z

@smarter Good point, as Product is not decomposable, the code path should be the same as before for the test V, as we didn't make any changes to the space algorithm.

liufengyun · 2017-09-08T22:38:06Z

Superseded by #3061

dottybot reviewed Sep 6, 2017

View reviewed changes

liufengyun force-pushed the fix-3015b branch from 7cd7fe9 to 3d073cd Compare September 6, 2017 08:27

odersky added 28 commits September 6, 2017 16:51

Adapt operations in Types

a060095

So far, everything up to parents is adapted to new scheme. Parents is half done; needs more work once we change layour of ClassInfo.

Move givenSelfType to ClassSymbol

fb08d5f

Simplifies usage and removes some obscure code in Types

Handle AppliedTypes

dabeeca

Systematically introduce handlers for AppliedTypes where there was a handler for HKApply.

Introduce TypeArgRef

a16f5fc

Also, change asSeenFrom to use it for arguments

Various fixes

14ef226

More fixes

0fdfd30

Fix stray brace

e4b9c30

Fix typing of _* arguments

d563cdf

Fix rebase breakage

9c3b90d

Fix debug output to make it more stable

70a6bcc

Avoid printing addresses and reduce junk.

Fix Stackoverflow in asSeenFrom

235ef90

We had a stackoverfloa in asSeenFrom even under the old scheme, since there was a bad change involving checkRealizableBounds.

Make newScheme non-final

62f612c

Otherwise we always need to a clean compile to update things.

Partially revert change in TypeApplications#Reducer

69226cd

Use atVariance for new cases in TypeMaps and TypeAccumulators

e675fee

Fix variance in avoidParams

5d0fe46

Need to avoid variance 0, as it leads to leaking ranges.

Fix isSubArg

3f8ee89

Strange as it sounds, a TypeBounds can be a subtype of a single type.

Avoid infinite expansion in normaizeWildcardArgs

b8f8b44

Handle parameters from base classes of package objects

7e4f591

Needed to compile i0239.scala correctly.

More fixes

b9b1bcd

Fix scala#536 again

6cd44c8

Test case was extended and several fixes were necessary to make it pass.

Adapt ClassTypeParamCreationFlags to new scheme

c0649b9

Re-fixes scala#938

Fix TypeArgRef and argForParam

08de872

- Several fixes for TypeArgRef. - argForParam needs to follow owner chains like toPrefix does (see t119.scala)

Add capture conversion

a225978

Needed to make i2250.scala compile

Fix ExpandSAMs

306d36c

Can't rely any ore that refinements are stripped away in normalizeClassRef

Fix implicit scope computation

68d6ddb

Refine typeMismatchMsg

fa2d7dd

Without the tweak, neg/i1181c.scala gives the non-sensical message `type mismatch: found: Foo[Int, Int], expected: Any`.

odersky and others added 11 commits September 6, 2017 16:51

Reorder and clean up erasure and sigName

e2703d3

More cleanups and removals of now redundant code

5fed255

Better implementation of mapArgs

6491ef2

Specialize hash-consing of WithFixedSym types

933c677

Avoid creating unnecessary new lists in mapArgs

656ba92

Drop unused RefType and ClassRef

9aafa1d

Remove unused code

778f4d1

Reverted: Refine Space#refine to handle AppliedTypes

c003228

scala#3054 has a better approach.

Disable exhaustivity test

410d0cf

reenable exhaustivity check

7b81aee

fix scala#3015: use type inference to type child classes

6e50ab3

liufengyun force-pushed the fix-3015b branch from 3d073cd to 7388f6c Compare September 7, 2017 09:37

make exhaustivity check work on native apply

7a49c8c

liufengyun force-pushed the fix-3015b branch from 7388f6c to 7a49c8c Compare September 7, 2017 10:50

liufengyun added 2 commits September 7, 2017 17:44

change var to val

2953cab

remove uselss expose

3bd360c

enable disabled test

33efe0f

odersky reviewed Sep 8, 2017

View reviewed changes

liufengyun closed this Sep 8, 2017

Fix #3015: exhaustivity check on top of native apply #3074

Fix #3015: exhaustivity check on top of native apply #3074

Uh oh!

Conversation

liufengyun commented Sep 6, 2017

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

liufengyun commented Sep 7, 2017

Uh oh!

dottybot commented Sep 7, 2017

Uh oh!

dottybot commented Sep 7, 2017

Uh oh!

liufengyun commented Sep 8, 2017

Uh oh!

odersky commented Sep 8, 2017

Uh oh!

liufengyun commented Sep 8, 2017

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

odersky Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

smarter commented Sep 8, 2017

Uh oh!

liufengyun commented Sep 8, 2017

Uh oh!

smarter commented Sep 8, 2017

Uh oh!

liufengyun commented Sep 8, 2017

Uh oh!

liufengyun commented Sep 8, 2017

Uh oh!

Uh oh!