Skip to content

Disable checkNoPrivateLeaks when unpickling #4133

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

Merged
merged 3 commits into from
Mar 29, 2018

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 17, 2018

No description provided.

I get:

undefined: Test.cs.apply(0).+ # 3005023: TermRef(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class mutable)),class IndexedSeqView)),type A),+) at frontend
one error found
    Compilation failed for: 'tests/run/viewtest.scala'

I have similar errors locally for another file "test4refine.scala" which passes the CI. This makes me believe that it is some compilation order dependence.
@odersky
Copy link
Contributor Author

odersky commented Mar 17, 2018

We should try to improve the FromTasty tests and the FromTasty compilation. Right now, we have many tests failing in a pattern that seems random because we don't get a lot of info what caused the failure. You have to go to the log file to see anything. Compare with CompilationTests where the output immediately tells you what went wrong.

The other problem is that FromTasty still fails too often. The fact that we have blacklists means that there are root causes that cause a test to fail and these root causes are sometimes still poorly understood. Anytime someone changes something, it could be that some of these root causes now also applies to some test (or to a new test) and causes it to fail. I had a lot of failures that way recently, which had nothing to do with the changes, but rather exposed another way to trigger one of the root causes. Tracking down and fixing the root causes would therefore help our productivity a lot.

@smarter
Copy link
Member

smarter commented Mar 17, 2018

Tracking down and fixing the root causes would therefore help our productivity a lot.

Agreed. Hence my quest to fix enough things to remove the blacklists over the past week ;).

@smarter
Copy link
Member

smarter commented Mar 17, 2018

I have similar errors locally for another file "test4refine.scala" which passes the CI. This makes me believe that it is some compilation order dependence.

test4refine.scala is broken on OS X due to case-insensitivity issues, this is fixed by the open PR #4120, the viewtest.scala failure is probably unrelated since it fails even on the Linux CI.

@odersky
Copy link
Contributor Author

odersky commented Mar 17, 2018

@smarter Yes, it's really good that you are doing this!

@odersky odersky requested a review from smarter March 17, 2018 20:38
@smarter smarter self-assigned this Mar 20, 2018
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the viewtest.scala crash is a real issue, it can be minimized to:

import scala.collection.mutable.IndexedSeqView

object Test {
  val zs: IndexedSeqView[Int, Array[Int]] = Array(1, 2, 3).view
  val cs = zs.reverse

  cs(0) += 1
}

If you replace cs(0) += 1 by val a = cs(0) you get a crash earlier in refchecks too. The issue is related to the type of cs which is inferred to zs.This even though This is declared as private[this] in https://github.com/scala/scala/blob/2.12.x/src/library/scala/collection/mutable/IndexedSeqView.scala

@smarter smarter assigned odersky and unassigned smarter Mar 20, 2018
@Blaisorblade
Copy link
Contributor

Ah, so def reverse: This is just short for def reverse: IndexedSeqView[A, Coll], and this must be expanded when compiling IndexedSeqView, since This is not in scope for clients.
The alternative would be to do the expansion when compiling users of IndexedSeqView, but that feels illegal.

In fact, private[this] members can't be refined, so it seems they can be replaced by their bounds according to variance (which also handles aliases elegantly). It seems this can be done even when typechecking their bodies, if more convenient:

class Foo {
  private[this] type A >: L <: U
  // use A in covariant/contravariant position.
  // Not sure what to do with invariant uses, do they make any sense?
  def f(x: A): A = {
    // can only assume x <: U
    // return value must be <: L
   ...body...
  }
  //that means
  //def f(x: L): U = {
  // ...body...
  //}
}

The member comes from a particular space but it has to be seen from the
current prefix to be valid. This is why compiling viewtest.scala from
TASTY stopped working two commits ago.
@smarter
Copy link
Member

smarter commented Mar 26, 2018

@odersky I managed to find the culprit, see latest commit.

@odersky
Copy link
Contributor Author

odersky commented Mar 29, 2018

@smarter Oops! Seems obvious in hindsight 😄

@odersky
Copy link
Contributor Author

odersky commented Mar 29, 2018

So we can merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants