Check method arguments with parametricity when static

[Xavientois]

Closes #14460, #14751

When a global static is called, allow for a cold argument if the corresponding parameter is not Matchable.

When merged, this PR will bring the number of safe-init warnings from bootstrapping the compiler down to 23.

[Xavientois]

@olhotak @liufengyun This PR replaces #14751. It is currently incomplete, as I am unsure how to check whether a param is a type variable. Additionally, it fails the added test due to the parameters of Array.apply being Matchable.

Do you have any suggestions regarding this?

[olhotak]

It shouldn't matter whether it's a type variable or not, only whether it's a subtype of Matchable or not. A type variable whose upper bound is a subtype of Matchable wouldn't be safe to consider parametric, while a non-type-variable parameter type of Any is parametric.

Which apply is it (Array has many of them)? In def apply[T : ClassTag](xs: T*): Array[T], the parameter type is T*, and T is not a subtype of Matchable.

[Xavientois]

It shouldn't matter whether it's a type variable or not, only whether it's a subtype of Matchable or not. A type variable whose upper bound is a subtype of Matchable wouldn't be safe to consider parametric, while a non-type-variable parameter type of Any is parametric.

I agree, but we previously discussed that an special case needed to be made in order to allow for calls to wrapRefArray as can be seen in the desugared code here.

Which apply is it (Array has many of them)? In def apply[T : ClassTag](xs: T*): Array[T], the parameter type is T*, and T is not a subtype of Matchable.

It appears to be this one.

[olhotak]

This should also be a fix for #9795.

[Xavientois]

@olhotak @liufengyun This solution does mean that we can pass cold values to println, such as with the test early-promote2.scala:

class M {
  println(this)       // error (This line no longer causes an error)
  foo()
  private val a = 5   // error
  def foo() = a
}

Should I update that test to allow this?

[liufengyun]

Should I update that test to allow this?

It seems to suggest we should always check for type parameters. This way, we can avoid unsafe usage like println.

[Xavientois]

It seems to suggest we should always check for type parameters. This way, we can avoid unsafe usage like println.

What sort of check are you picturing? The methodType for println is

MethodType(List(x), List(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any)), TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Unit))

[liufengyun]

It seems to suggest we should always check for type parameters. This way, we can avoid unsafe usage like println.

What sort of check are you picturing? The methodType for println is

MethodType(List(x), List(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any)), TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Unit))

I suggest always check paraInfo.isInstanceOf[TypeRef] && paraInfo.symbol.isTypeParam && !(paraInfo <: defn.MatchableType)

[olhotak]

Just so we're clear, in theory, requiring the parameter type to be a type variable wouldn't be any more sound than just allowing Any, so we're talking about a heuristic. For example, the following compiles:

def myPrintln[T](x: T) = println(x)

Then I can bypass any warning on println by calling myPrintln instead.

That said, allowing initialization to skip parametric parameters only if their types are type variables may well be a good heuristic. One worry I'd have is explaining it to users: if you do or don't get a warning in a certain case, how do you explain to users why you are/are not supposed to get a warning?

For now, since @Xavientois has limited time, I propose to implement it and we can revisit the heuristic later.

[Xavientois]

I suggest always check paraInfo.isInstanceOf[TypeRef] && paraInfo.symbol.isTypeParam && !(paraInfo <: defn.MatchableType)

@liufengyun After making the change, we are now back to failing the cases this PR was originally made to handle. If I use "or" rather than "and", it behaves exactly the same as if the isTypeParam clause was not there. Is that clause incorrect?

[liufengyun]

I suggest always check paraInfo.isInstanceOf[TypeRef] && paraInfo.symbol.isTypeParam && !(paraInfo <: defn.MatchableType)

@liufengyun After making the change, we are now back to failing the cases this PR was originally made to handle. If I use "or" rather than "and", it behaves exactly the same as if the isTypeParam clause was not there. Is that clause incorrect?

Do you mean the enum test case? The condition looks reasonable, it's hard to tell how we should change it. It would be nice to debug why the check is false.

[Xavientois]

It would be nice to debug why the check is false.

@liufengyun It looks like, for println and apply, it is not a type param, thus causing the tests to fail.

The tests in question are:

• tests/init/pos/inner-enum-multi-variant.scala
• tests/init/pos/inner-enum.scala
• tests/init/pos/enum-ordinal.scala
• tests/init/pos/enum-desugared.scala
• tests/init/pos/inner-case.scala
• tests/init/pos/some-this.scala
• tests/init/pos/inner-new.scala

If I try to change it from isTypeParam && !isMatchable to isTypeParam || !isMatchable, it causes a number of negative tests to fail.

[liufengyun]

@liufengyun It looks like, for println and apply, it is not a type param, thus causing the tests to fail.

That is kind of expected, I think we do want to warn in those cases.

For other failing tests, they need to be analyzed case by case. If you are time-constrained, I'd suggest leaving the PR as it is. @olhotak and I can take it over. There are some design changes of parametricity coming up during our last meeting.

[Xavientois]

That is kind of expected, I think we do want to warn in those cases.
I don't think we want to warn forapply since Array.apply, List.apply, etc. since they were the specific cases that prompted the need for parametricity in the first place.

The individual cases that fail due to parametricity that I am uncertain about are the following:

• tests/init/neg/early-promote.scala
• tests/init/neg/by-name-error.scala
• tests/init/neg/inlined-method.scala
• tests/init/neg/inner25.scala
• tests/init/neg/early-promote2.scala
• tests/init/neg/inner11.scala
• tests/init/neg/promotion-loop.scala
• tests/init/neg/scodec.scala
• tests/init/neg/leak-warm.scala
• tests/init/neg/inner-loop.scala
• tests/init/neg/t3273.scala

@liufengyun Above, you mentioned:

It seems to suggest we should always check for type parameters. This way, we can avoid unsafe usage like println.

I think that this will help with many of these test cases, but we would need to find another way than using isTypeParam, since that breaks the test cases that we want to pass (the enum ones based on Array.apply)

[liufengyun]

I think that this will help with many of these test cases, but we would need to find another way than using isTypeParam, since that breaks the test cases that we want to pass (the enum ones based on Array.apply)

For Array.apply and List.apply, it's strange that isTypeParam is not true, as those methods do have a type parameter T.

[Xavientois]

@liufengyun I figured out the problem. We needed to do info.repeatedToSingle.isInstanceOf[TypeParamRef].

The only remaining test to figure out is cold-insert-hot-array. It was added in #14895 to prevent this case from being accepted, but the current parametricity approach permits this code. The case is as follows:

object A:
  def foo[T](x: T, array: Array[T]): Unit = array(0) = x

class B {
  var a = new Array[B](2)
  A.foo(this, a) // error
  println(a(0).i)
  val i = 99
}

Should I remove this test case in order to merge this PR or should we figure out a way to trigger a warning for this case?

[liufengyun]

@liufengyun I figured out the problem. We needed to do info.repeatedToSingle.isInstanceOf[TypeParamRef].

Great you figured it out 👍 The argument is varargs, thus we need to get the component. Here indeed we cannot test the symbol, because in Scala 3 the method type uses TypeParamRef and ParamRef instead of symbols --- the parameter symbols are only visible inside the method (Sorry for the incorrect code snippet).

For the failing test you mentioned, that's exactly the purpose of it: to defend against unsound solutions. The solution @olhotak and I discussed is the following:

• A non-hot method argument is allowed if the corresponding parameter type is a type parameter T with Any as its upper bound and Nothing as its lower bound.
• For any non-hot argument allowed for a type parameter T, the other arguments should either correspond to a parameter type that is T or that does not contain T as a component.
• If any non-hot method argument is allowed above, the result value of the method call is assumed to be Cold.

The method Type.existsPart will be useful for the 2nd test.

[Xavientois]

@liufengyun Just tried the approach you suggested. It does fix that failing cold-insert-hot-array test case but also breaks the following four (the cases embodying the original motivator for this PR):

• tests/init/pos/enum-desugared.scala
• tests/init/pos/inner-enum.scala
• tests/init/pos/inner-enum-multi-variant.scala
• tests/init/pos/enum-ordinal.scala

The problem is that the signature of the apply method has an Applied Type as seen here: https://www.scala-lang.org/api/current/scala/Array$.html#apply[T](xs:T*)(implicitevidence$5:scala.reflect.ClassTag[T]):Array[T]

def apply[T](xs: T*)(implicit arg0: ClassTag[T]): Array[T]

The implicit arg0 breaks the second condition you outlined:

For any non-hot argument allowed for a type parameter T, the other arguments should either correspond to a parameter type that is T or that does not contain T as a component.

Any ideas on how to work around this?

[liufengyun]

The implicit arg0 breaks the second condition you outlined:

For any non-hot argument allowed for a type parameter T, the other arguments should either correspond to a parameter type that is T or that does not contain T as a component.

Any ideas on how to work around this?

Thanks for trying it. In this case, we can specialize the logic to allow all arguments of the type ClassTag[?].

[liufengyun]

Well done @Xavientois 🎉

I left some comments for your reference.

[liufengyun]

LGTM, thank you @Xavientois 🎉

[olhotak]

LGTM