Don't force NamedType denotations in containsRefinedThis #1026

odersky · 2016-01-14T11:12:22Z

containsRefinedThis inspects symbols and infos of named types in order
to avoid needless traversals. As i974 shows, this can lead to infinite
recursions. The fix is not to force a NamedType denotation, assume the
worst and traverse the prefix if a NamedType is not yet populated
with a denotation.

Fixes #974 and makes MutableSortedSetFactory in stdlib compile.

Review by @smarter

odersky · 2016-01-14T12:14:25Z

/rebuild

odersky · 2016-01-14T12:14:59Z

It was a GC overhead limit exceeded again. Is there a way to increase the heap or alternatively reduce the level of parallelism?

odersky · 2016-01-14T13:24:03Z

Out of heap space this time. We need to change the build script to take more heap. Can someone do this please?

smarter · 2016-01-14T16:05:10Z

I think this PR only addresses the symptoms and not the root of the problem, which is that we do not check for cycles in the prefixes of TypeRefs which are RefinementTypes. For example, the following causes an infinite loop instead of printing illegal cyclic reference: upper bound Bar[T]#Elem of type T refers back to the type itself:

trait Foo[T <: Bar[T]#Elem]
trait Bar[T] {
  type Elem = T
}

odersky · 2016-01-14T16:17:23Z

The two problems are unrelated. #974 caused the StackOverflow before the
cycle check was even run.
@smarter, can you make this new failing test into a separate issue?

On Thu, Jan 14, 2016 at 5:05 PM, Guillaume Martres <[email protected]

wrote:

I think this PR only addresses the symptoms and not the root of the
problem, which is that we do not check for cycles in the prefixes of
TypeRefs which are RefinementTypes. For example, the following causes an
infinite loop instead of printing illegal cyclic reference: upper bound
Bar[T]#Elem of type T refers back to the type itself:

trait Foo[T <: Bar[T]#Elem]trait Bar[T] {
type Elem = T
}

—
Reply to this email directly or view it on GitHub
#1026 (comment).

Martin Odersky
EPFL

smarter · 2016-01-14T16:23:04Z

Are you sure? If I apply the following on top of master: https://github.com/dotty-staging/dotty/commits/try/refinement-cycles then tests/pos/i974.scala succeeds as well as my example above (however, other tests like ./tests/pos/tcpoly_ticket2096.scala now fail)

odersky · 2016-01-14T18:13:55Z

@smarter Maybe you are right. I fixed checkNonCyclic and your example compiles OK (i.e. with errors) now.

odersky · 2016-01-14T18:44:35Z

The heap problem has gone away. We are back to the previous

pos/printing: compilation failed

without any further diagnostic. It's super urgent that we fix this, no progress is possible without it.

smarter · 2016-01-14T20:59:52Z

/rebuild

smarter · 2016-01-14T21:00:55Z

@odersky : Maybe try rebasing on master where the partest jenkins job does not use bootstrapped dotty anymore.

DarkDimius · 2016-01-14T21:01:21Z

Ok. I was able to get to the archives of workspace.
But the log does no say much. eg, for
https://scala-ci.typesafe.com/job/dotty-master-validate-partest/906/console

where printing-pos failed, the compiler output only contains:

compiling /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/Disambiguation.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/PlainPrinter.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/Printer.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/Printers.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/RefinedPrinter.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/Showable.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/Texts.scala /home/jenkins/workspace/dotty-master-validate-partest/tests/partest-generated/pos/printing/package.scala
options: -Xprint-types -Ytest-pickler -Ystop-after:pickler -pagewidth 160 -Yno-deep-subtypes -Yno-double-bindings -d ./tests/partest-generated/pos/printing-pos.obj -Ycheck:tailrec,resolveSuper,mixin,restoreScopes,labelDef

Not informative.

DarkDimius · 2016-01-14T21:02:35Z

To get the archive of workspace:
https://scala-ci.typesafe.com/job/dotty-master-validate-partest/906/ than click the Build Artifacts link.
It should have a *.clog file for every failed test:

dark@reco ~/Downloads/archive 2 $ find . -iname "*.clog"
./tests/partest-generated/pos/printing-pos.clog

smarter · 2016-01-14T22:01:38Z

@smarter Maybe you are right. I fixed checkNonCyclic and your example compiles OK (i.e. with errors) now.

So should we revert the original fix since it's not actually needed?

smarter · 2016-01-14T22:02:42Z

tests/neg/i974.scala

+trait Bar[T] {
+  type Elem = T
+}
+trait Foo2[T <: Bar2[T]#Elem] // error: illegal cyclic reference


Did you intend to have the same test twice, with just the names changed? (Foo -> Foo2, Bar -> Bar2)

Yes, because 1 expected error also succeeds if the file is not found.

odersky · 2016-01-14T22:30:07Z

@smarter I think the checkNonCyclic fix is worth doing anyway.

smarter · 2016-01-14T22:39:08Z

@smarter I think the checkNonCyclic fix is worth doing anyway.

You mean the containsRefinedThis fix, right? Anyway, LGTM once the tests pass.

smarter · 2016-01-14T22:56:05Z

Ok. I was able to get to the archives of workspace
But the log does no say much

@DarkDimius : looking at https://github.com/lampepfl/dotty/blob/master/test/dotty/partest/DPDirectCompiler.scala#L29 I see that it's trying to redirect the output by using a custom Context, but using a custom Context is currently broken, so I don' t think that can work: https://github.com/lampepfl/dotty/blob/da661c8a509efaa1c0ff926b0907c32cd37708a5/src/dotty/tools/dotc/Driver.scala#L37-L39
It also won't include the output of calls to println, etc. It might be simpler to just always redirect stdout and stderr using https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#setOut-java.io.PrintStream- and https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#setErr-java.io.PrintStream-

smarter · 2016-01-14T23:13:42Z

Anyway, LGTM once the tests pass

Actually, there's a missing change: remove the comment at https://github.com/lampepfl/dotty/pull/1026/files#diff-2fe35c59082b97d7128120593f9ec2e7R172 about #Apply which is no longer accurate.

odersky · 2016-01-14T23:20:38Z

/rebuild

containsRefinedThis inspects symbols and infos of named types in order to avoid needless traversals. As i974 shows, this can lead to infinite recursions. The fix is not to force a NamedType denotation, assume the worst and traverse the prefix if a NamedType is not yet populated with a denotation. Fixes scala#974 and makes MutableSortedSetFactory in stdlib compile.

Previous jenkins run did not run out of memory but ran very slowly, indicating trashing. Trying with half the heap size.

Leaving some last stop to print something even for fatal exceptions.

Need to also look info refined types. Need to handle case where we hit a NoCompleter again.

I observed in a local partest a file with was a java.io.Path, not an SFile. They should be treated like SFiles. Not clear why this came up. The file in question (partest-generated/pos/Patterns_v1.scala) looked just like all the others that were read as SFiles.

(reverted from commit a7bc089)

odersky · 2016-01-15T16:54:58Z

Superseded by #1028

smarter reviewed Jan 14, 2016
View reviewed changes

odersky added 11 commits January 15, 2016 16:08

Try to increase heap size to 3G

95bc68d

Reduce heap size to 1500M

7dbe08f

Previous jenkins run did not run out of memory but ran very slowly, indicating trashing. Trying with half the heap size.

Massage exception handling in TreeChecker

8a3d46b

Leaving some last stop to print something even for fatal exceptions.

Fix heapsize setting

c6ba90f

Fix checkNonCyclic

e5b9caa

Need to also look info refined types. Need to handle case where we hit a NoCompleter again.

Disable pos/printing

ab50847

Revert: Disable pos/printing

00ee7e6

(reverted from commit a7bc089)

disable testy_dotc_printing instead

3ea01f6

disable all printing tests

03bd363

Disable only tasty_printing

2a5dd4e

odersky force-pushed the fix-#974 branch from 1dc8636 to 2a5dd4e Compare January 15, 2016 15:13

smarter mentioned this pull request Jan 15, 2016

Change early typeparams #1027

Closed

odersky closed this Jan 15, 2016

Don't force NamedType denotations in containsRefinedThis #1026

Don't force NamedType denotations in containsRefinedThis #1026

Uh oh!

Conversation

odersky commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

DarkDimius commented Jan 14, 2016

Uh oh!

DarkDimius commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

smarter Jan 14, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Jan 14, 2016

Choose a reason for hiding this comment

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

smarter commented Jan 14, 2016

Uh oh!

odersky commented Jan 14, 2016

Uh oh!

odersky commented Jan 15, 2016

Uh oh!

Uh oh!