Fix #5948: Be more careful with capturing #5957

odersky · 2019-02-20T21:21:20Z

No description provided.

odersky · 2019-02-21T15:52:25Z

Comment on last commit:

Delete DottyByteCodeTest

I am giving up. This is the worst sort of blindly testing the status quo! It tests some random
code and if that code has changed (because there is some optimization applied now) it fails.
Great! Problem is there's no way to automatically update the code. One has to go in manually and
fix the expected program including all labels! Copy pasting from the diff does not work and there's no
--update-check.

As long as things are like this I am afraid that bytecode tests are throwaway tests. They
blindly test the status quo, and if that changes our best action is to throw away the test.

The problem is that while the old test indeed tested nothing of value, a new version of it would
test something interesting: That an unused cast is eliminated. But it's just too hard to update the test, so as long as the testing framework is not upgraded to be more user friendly, these tests are
not as useful as they could be.

- it should not be Erased since that will remove the complete call, including the expression that is casted. It should be StableRealizable, so that a pure expression followed by a synthesized cast is still recognized as pure. - to make up for the loss of Erased, we take the method into account specifically when deciding whether a tree is erased.

Replace all instances of compiler-generated type casts by calls to `Any_typeCast` via `tpd.cast`. This will assume that all these calls are pure.

If tree is not a subtype of expected type, try to capture wildcard types in its type with fresh skolems.

Capture conversion is generally sound only if the LHS of a comparison is a path.

The previous code for `ModuleSerializationProxyConstructor` required capture conversion to work.

I am giving up. This is the worst sort of blindly testing the status quo! It tests some random code and if that code has changed (because there is some optimization applied now) it fails. Great! Problem is there's no way to automatically update the code. One has to go in manually and fix the expected program including all labels! Copy pasting from the diff does not work and there's no --update-check. As long as things are like this I am afraid that bytecode tests are throwaway tests. They blindly test the status quo, and if that changes our best action is to throw away the test.

compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala

It seems this is necessary for code generation. We got abstract method errors in typer/TermRefSet otherwise: ``` java.lang.AbstractMethodError: Method dotty/tools/dotc/typer/TermRefSet$$anon$1.accept(Ljava/lang/Object;Ljava/lang/Object;)V is abstract, took 2.601 sec [error] at dotty.tools.dotc.typer.TermRefSet$$anon$1.accept(Implicits.scala) [error] at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684) [error] at dotty.tools.dotc.typer.TermRefSet.foreach(Implicits.scala:1638) [error] at dotty.tools.dotc.typer.TermRefSet.$plus$plus$eq(Implicits.scala:1634) ```

compiler/src/dotty/tools/dotc/transform/Erasure.scala

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

compiler/src/dotty/tools/dotc/core/Types.scala

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

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

library/src-bootstrapped/scala/internal/TypeBox.scala

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

@allanrenucci

Reinstantiate DottyByteCodeTest with the change proposed by @allanrenucci. This reverts commit e04918e.

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

odersky · 2019-02-21T22:00:48Z

We now have a much simpler conversion scheme e05c1a7, where we just need to skolemize the type of a tree as a whole, no need to go replace parameters by TypeBoxes.

odersky · 2019-02-21T22:14:41Z

test performance please

dottybot · 2019-02-21T22:16:29Z

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

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

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

dottybot · 2019-02-22T01:22:43Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (daf4ce9)

Test case pos/capture.scala contains an explanation why a different scheme we tried does not work.

odersky · 2019-02-22T10:00:12Z

Unfortunately the simplified scheme failed a test: hmm-shonan/Lifters.scala. I have therefore reverted to the old scheme with TypeBoxes.

The essence of the failure is condensed in test pos/capture.scala. That test also explains the difference between the two schemes I have tried.

odersky · 2019-02-22T10:02:16Z

test performance please

dottybot · 2019-02-22T10:02:40Z

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

dottybot · 2019-02-22T11:49:48Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (daf4ce9)

A SkolemType is not a symbol whose info is transformed separately. That's why mapping a SkolemType must involve mapping its info. A test case is the new version of pos_valueclasses/t7818.scala.

Since we now map the info of a SkolemType as part of mapping the SkolemType itself, it follows that SkolemTypes can't recursively refer to themselves. So `captureWildcards` in `Typer` avoids recursion now by using an approximating substitution instead (I wonder now how we ever could live without approximating substitutions!). This means that SkolemTypes don't need to be mutable anymore since there is no fixed point to construct.

odersky · 2019-02-22T14:01:30Z

test performance please

dottybot · 2019-02-22T14:02:12Z

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

dottybot · 2019-02-22T17:07:03Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (fda1250)

odersky · 2019-02-22T17:52:44Z

Here's the status:

I replaced the previous unsound capture conversion by two measures:

Apply capture conversion in isSubArgs only if the LHS is a path (then it is sound)
Skolemize wildcards in adapt if a tree does not conform to its expected type.

(2) alone would be sufficient, i.e. all tests pass with it. But performance tests indicate that it's better to also do (1). And, besides, (1) is in a sense less intrusive since we do not need to skolemize. So we should do it when we can.

odersky · 2019-02-23T11:07:42Z

@smarter I am done now. Can you give it another look?

smarter

Your comment in Fix #5948: Be more careful with capturing #5957 (comment) explaining how capture conversion is actually not required but it makes sense to have it here anyway is informative, I think this information should be preserved in a comment in the code too.
I still think we could avoid having leftRoot be a mutable variable by nesting isSubArgs in isSubType, but maybe there's a good reason not to do that?

smarter · 2019-02-23T11:50:52Z

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

@@ -2697,10 +2698,47 @@ class Typer extends Namer
      case tree: Closure => cpy.Closure(tree)(tpt = TypeTree(pt)).withType(pt)
    }

+    /** Replace every top-level occurrence of a wildcard type argument by


I'm not 100% clear on the notion of "top-level occurence" here, why is in an occurence in a branch of an AndOrType top-level, but not in the widened type of an ExprType for example ?

It's because we have already widened the type before we pass it to captureWildcards.

odersky · 2019-02-23T13:31:04Z

I still think we could avoid having leftRoot be a mutable variable by nesting isSubArgs in isSubType, but maybe there's a good reason not to do that?

Yes, it's because that way we don't thread a lot of parameters through all recursive calls (in the implementation, not the source). That's why recur is outside isSubArgs.

I'll make a note explaining that.

odersky force-pushed the fix-#5948 branch from 795a6f9 to 424d5ea Compare February 21, 2019 08:34

odersky added 13 commits February 21, 2019 17:13

Systematically use cast in compiler

bdfc5a9

Replace all instances of compiler-generated type casts by calls to `Any_typeCast` via `tpd.cast`. This will assume that all these calls are pure.

Replace TypeBound arguments by wildcards in wildApprox

237eba0

Update decompiled files to account for new type tests

2b02931

Introduce capture wildcard adaptation

086a218

If tree is not a subtype of expected type, try to capture wildcard types in its type with fresh skolems.

New method substApprox on types

c9be032

Drop capture conversion in type comparer

7410682

Capture convert only if LHS is a path

f1ab7dc

Capture conversion is generally sound only if the LHS of a comparison is a path.

Fix Definitions

7165b98

The previous code for `ModuleSerializationProxyConstructor` required capture conversion to work.

Fix bytecode test

6d21081

Add missing library file

545cfe7

Fix bytecode tests some more

3553f4e

allanrenucci reviewed Feb 21, 2019

View reviewed changes

compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala Outdated Show resolved Hide resolved

odersky force-pushed the fix-#5948 branch from 5302373 to 83ac988 Compare February 21, 2019 16:35

smarter requested changes Feb 21, 2019

View reviewed changes

odersky added 3 commits February 21, 2019 18:53

Reinstantiate DottyByteCodeTest

cee8162

Reinstantiate DottyByteCodeTest with the change proposed by @allanrenucci. This reverts commit e04918e.

Drop dead code

9e16a48

Utility functions to test whether a type is a TypeBounds

abb8146

smarter reviewed Feb 21, 2019

View reviewed changes

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved

odersky force-pushed the fix-#5948 branch from 8c12eea to 175f8cc Compare February 21, 2019 21:58

smarter reviewed Feb 21, 2019

View reviewed changes

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/core/TypeComparer.scala Show resolved Hide resolved

compiler/src/dotty/tools/dotc/core/TypeComparer.scala Show resolved Hide resolved

Document capture conversion in TypeComparer and Typer

78939ca

Test case pos/capture.scala contains an explanation why a different scheme we tried does not work.

odersky force-pushed the fix-#5948 branch from 175f8cc to 78939ca Compare February 22, 2019 09:38

Document TypeBox and move to library/src

79bfff4

Address remaining review comments

100bdeb

odersky added 3 commits February 22, 2019 14:27

Transform infos of SkolemTypes in TypeMap

788fac8

A SkolemType is not a symbol whose info is transformed separately. That's why mapping a SkolemType must involve mapping its info. A test case is the new version of pos_valueclasses/t7818.scala.

Print info of SkolemTypes under -Xprint-types

6e811b4

odersky force-pushed the fix-#5948 branch from 113ee1c to 6e811b4 Compare February 22, 2019 13:37

smarter approved these changes Feb 23, 2019

View reviewed changes

Document two aspects suggested by review

d4306dd

odersky merged commit e777f34 into scala:master Feb 23, 2019

Blaisorblade deleted the fix-#5948 branch February 24, 2019 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5948: Be more careful with capturing #5957

Fix #5948: Be more careful with capturing #5957

odersky commented Feb 20, 2019

odersky commented Feb 21, 2019 •

edited

Loading

odersky commented Feb 21, 2019

odersky commented Feb 21, 2019

dottybot commented Feb 21, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019 •

edited

Loading

odersky commented Feb 22, 2019

dottybot commented Feb 22, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019

dottybot commented Feb 22, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019

odersky commented Feb 23, 2019

smarter left a comment

smarter Feb 23, 2019

odersky Feb 23, 2019

odersky commented Feb 23, 2019 •

edited

Loading

Fix #5948: Be more careful with capturing #5957

Fix #5948: Be more careful with capturing #5957

Conversation

odersky commented Feb 20, 2019

odersky commented Feb 21, 2019 • edited Loading

odersky commented Feb 21, 2019

odersky commented Feb 21, 2019

dottybot commented Feb 21, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019 • edited Loading

odersky commented Feb 22, 2019

dottybot commented Feb 22, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019

dottybot commented Feb 22, 2019

dottybot commented Feb 22, 2019

odersky commented Feb 22, 2019

odersky commented Feb 23, 2019

smarter left a comment

Choose a reason for hiding this comment

smarter Feb 23, 2019

Choose a reason for hiding this comment

odersky Feb 23, 2019

Choose a reason for hiding this comment

odersky commented Feb 23, 2019 • edited Loading

odersky commented Feb 21, 2019 •

edited

Loading

odersky commented Feb 22, 2019 •

edited

Loading

odersky commented Feb 23, 2019 •

edited

Loading