Skip to content
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

Drop requirement that self types are closed #16648

Merged
merged 3 commits into from
Jan 12, 2023
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 10, 2023

#702 introduced a requirement that self types are closed. This means

If trait X has self type S and C is a class symbol of S, then S also conforms
to the self type of C.

An example that violates this requirement is

trait X { self: Y => } // error: missing requirement: self type Y & X of trait X does not conform to self type Z of required trait Y
trait Y { self: Z => }
trait Z

But it's no longer clear what the requirement should achieve. If we let the example above typecheck and try to implement X with something like

class C extends X, Y

we would at that point get an error saying that C does not conform to the self type Z of Y. So it would have to be

class C extends X, Y, Z

and this one looks fine.

The original change in #702 was made to avoid a crash in pending/run/t7933.scala. Unfortunately, we cannot reproduce this anymore since it depends on nsc.interpreter, which is no longer part of Scala 2.13.

Since we are no longer sure what the restriction should achieve I think it's better to drop it for now. If people discover problems with code that uses "open" self types, we can try to fix those problems, and if that does not work, would fallback re-instituting the restriction. It's not ideal, but I don't see another way.

Fixes #16407

@odersky
Copy link
Contributor Author

odersky commented Jan 10, 2023

If someone manages to revive t7933.scala that would be very helpful. It's in pending/run. Here is the code:

mport scala.tools.nsc.interpreter.IMain

object Test extends App {
  val engine = new IMain.Factory getScriptEngine()
  engine.asInstanceOf[IMain].settings.usejavacp.value = true
  val res2 = engine.asInstanceOf[javax.script.Compilable]
  res2 compile "8" eval()
  val res5 = res2 compile """println("hello") ; 8"""
  res5 eval()
  res5 eval()
}

@sjrd
Copy link
Member

sjrd commented Jan 10, 2023

I guess the intention is to protect against situations like this:

trait X { self: Y =>
  val a = g() // what's the type of this val!?
  f(a)
}
trait Y { self: Z =>
  type B = A
  def f(a: B): Unit = ()
  def g(): A = ???
}
trait Z {
  type A
}

When computing the type of B as seen from X.this, we can end up with X.this.A. What does that mean when the self type of X is X & Y, which does not have a type member A?

@odersky
Copy link
Contributor Author

odersky commented Jan 10, 2023

@sjrd Ah yes, I think that was the crucial counter-example. But we are already dealing with it gracefully:

-- Error: i16407.scala:2:2 -----------------------------------------------------
2 |  f(g())
  |  ^
  |cannot resolve reference to type (X.this : Y & X).A
  |the classfile defining the type might be missing from the classpath
  |or the self type of (X.this : Y & X) might not contain all transitive dependencies
-- Error: i16407.scala:2:4 -----------------------------------------------------
2 |  f(g())
  |    ^
  |cannot resolve reference to type (X.this : Y & X).A
  |the classfile defining the type might be missing from the classpath
  |or the self type of (X.this : Y & X) might not contain all transitive dependencies
2 errors found

So I think we don't need a separate test in RefChecks (which would come later anyway).

@odersky odersky force-pushed the fix-16407 branch 2 times, most recently from 58d68ae to 60cb909 Compare January 11, 2023 10:29
@odersky odersky requested a review from sjrd January 11, 2023 19:24
scala#702 introduced a requirement that self types are closed. This means

> If trait X has self type S and C is a class symbol of S, then S also conforms
to the self type of C.

An example that violates this requirement is
```scala
trait X { self: Y => } // error: missing requirement: self type Y & X of trait X does not conform to self type Z of required trait Y
trait Y { self: Z => }
trait Z
```
But it's no longer clear what the requirement should achieve. If we let the example above
typecheck and try to implement X with something like
```scala
class C extends X, Y
```
we would at that point get an error saying that `C` does not conform to the self type Z of Y.
So it would have to be
```scala
class C extends X, Y, Z
```
and this one looks fine.

The original change in scala#702 was made to avoid a crash in pending/run/t7933.scala.
Unfortunately, we cannot reproduce this anymore since it depends on nsc.interpreter,
which is no longer part of Scala 2.13.

Since we are no longer sure what the restriction should achieve I think it's better to
drop it for now. If people discover problems with code that uses "open" self types, we
can try to fix those problems, and if that does not work, would fallback re-instituting
the restriction. It's not ideal, but I don't see another way.
}

trait Z[A] extends X {
self: Z[A] => // comment this to compile successfully
Copy link
Member

Choose a reason for hiding this comment

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

The comments in this pos file are not accurate. They say there's an error but clearly there isn't.

@@ -0,0 +1,8 @@

trait X { self: Y => } // error: missing requirement: self type Y & X of trait X does not conform to self type Z of required trait Y
Copy link
Member

Choose a reason for hiding this comment

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

Same here. There is no error anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. But we do leave the old errors in anyway if a neg test gets moved to pos as a documentation what went wrong before. That's done for many other pos tests as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK. I did not know that. All good, then.

@odersky odersky merged commit a517afb into scala:main Jan 12, 2023
@odersky odersky deleted the fix-16407 branch January 12, 2023 16:08
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
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.

Self type without type ascription introduces type requirements
3 participants