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

Regression(?) in kubukoz/slick-effect - type projections in separate compilation runs #22581

Closed
jchyb opened this issue Feb 11, 2025 · 14 comments
Assignees
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:wontfix

Comments

@jchyb
Copy link
Contributor

jchyb commented Feb 11, 2025

OpenCB: https://github.com/VirtusLab/community-build3/actions/runs/13209505213/job/36881103468

Compiler version

Working(?): 3.6.3
Broken(?): 3.6.4-RC1

Minimized code

Has to be split into 2 modules/separate compilation runs, for simplicity let's use scala-cli:

main.scala

package slickeffect

object Transactor {
  def fromDatabase(dbF: JdbcBackend#Database): Unit = ???
}

trait JdbcProfile {
  val backend: JdbcBackend = ??? 
}
object H2Profile extends JdbcProfile

trait JdbcBackend {
  type Database = DatabaseFactory
  val Database: DatabaseFactory = ???

  class DatabaseFactory {
    def forURL(url: String): DatabaseFactory = ???
  }
}

main.test.scala

package slickeffect

class TransactorTests {
  Transactor.fromDatabase(H2Profile.backend.Database.forURL("jdbc:h2:mem:"))
}

scala-cli test main.scala main.test.scala

Output

In 3.6.4-RC1 and before - no error.
In 3.7.0-RC1-bin-20250207-d60a914-NIGHTLY:

[error] -- [E007] Type Mismatch Error: /Users/jchyb/workspace/slick-effect/transactor/src/main/scala/slickeffect/Transactor.scala:28:61 
[error] 28 |    Transactor.fromDatabase(H2Profile.backend.Database.forURL("jdbc:h2:mem:"))
[error]    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |                 Found:    slickeffect.H2Profile.backend.DatabaseFactory
[error]    |                 Required: slickeffect.JdbcBackend#Database
[error]    |
[error]    | longer explanation available when compiling with `-explain`

Expectation

Not sure. Type projections like this should be unsupported in scala 3, and this error can also appear in previous compiler versions if both files are a part of the same compilation run. This leads me to believe this is more of a bugfix then regression, but I still wanted to log somewhere the OpenCB error and minimization.

@jchyb jchyb added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 11, 2025
@Gedochao Gedochao added area:typer stat:needs bisection Need to use nightly builds and git bisect to find out the commit where this issue was introduced and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 11, 2025
@jchyb
Copy link
Contributor Author

jchyb commented Feb 11, 2025

Errors in melivinlou/hammer-scala look similar https://github.com/VirtusLab/community-build3/actions/runs/13209436977/job/36880173932
EDIT: I no longer think this is true, see #22585

@prolativ
Copy link
Contributor

@jchyb Are you sure JdbcBackend#Database should be an illegal type projection? I'm looking at the specs https://docs.scala-lang.org/scala3/reference/dropped-features/type-projection.html and to me it's not quite clear if a trait should be considered an abstract type. Even if so, according to this specification and assuming I didn't miss something, changing JdbcBackend from a trait to a class should make this compile, but with the newest compiler this doesn't work either so IMO this is a regression anyway

@Gedochao Gedochao added the regression This worked in a previous version but doesn't anymore label Feb 11, 2025
@jchyb
Copy link
Contributor Author

jchyb commented Feb 11, 2025

You are right, it also errors out with a class. What bothered me mainly was the fact that in 3.6.3 and earlier this error did appear, but only if both the type definition and method call were in the same compilation run, which led to to think the error might be correct (although, If I recall correctly, the compiler should just inform us directly if an illegal type projection is used).

@jchyb
Copy link
Contributor Author

jchyb commented Feb 12, 2025

Same issue in tminglei/slick-pg: https://github.com/VirtusLab/community-build3/actions/runs/13209505213/job/36880970357
Clearer minimisation (same issue as before, has to be split into 2 compilation runs).
main.scala:

case class Test()(implicit proof: JdbcBackend#StreamingContext <:< JdbcBackend#Context)

class JdbcBackend:
  type Context = JdbcActionContext
  type StreamingContext = JdbcStreamingActionContext

  class JdbcStreamingActionContext extends JdbcActionContext
  class JdbcActionContext

main.test.scala:

val _ = new Test()

@Gedochao
Copy link
Contributor

Gedochao commented Mar 14, 2025

Last good release: 3.6.4-RC1-bin-20241121-5d1d274-NIGHTLY
First bad release: 3.6.4-RC1-bin-20241122-64411b6-NIGHTLY

26ecda5 is the first bad commit
cc @dwijnand @jchyb

@Gedochao Gedochao removed the stat:needs bisection Need to use nightly builds and git bisect to find out the commit where this issue was introduced label Mar 14, 2025
@Gedochao Gedochao assigned dwijnand and mbovel and unassigned dwijnand Mar 14, 2025
@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

I tried to further minimized and got the following single-file reproduction. Is that still the same problem?

// a.scala
package slickeffect

def fromDatabase(dbF: JdbcBackend#Database): Unit = ???

class JdbcBackend:
  type Database = DatabaseFactory
  val Database: DatabaseFactory = ???
  class DatabaseFactory

@main def main =
  val backend = JdbcBackend()
  fromDatabase(new backend.Database())
➜  ~/scala-snippets-6/22581 scala a.scala        
Compiling project (Scala 3.5.1, JVM (21))
[error] ./a.scala:12:16
[error] Found:    backend.DatabaseFactory
[error] Required: slickeffect.JdbcBackend#Database
[error]   fromDatabase(new backend.Database())
[error]                ^^^^^^^^^^^^^^^^^^^^^^
Error compiling project (Scala 3.5.1, JVM (21))
Compilation failed

@prolativ
Copy link
Contributor

prolativ commented Apr 7, 2025

@mbovel This seems like another issue: the reported problem is a regression from 3.6.3, while your snippet doesn't seem to compile in any version of scala 3. Just to double-check I rewrote it to something scala-2-compatible:

package slickeffect

class JdbcBackend {
  type Database = DatabaseFactory
  val Database: DatabaseFactory = ???
  class DatabaseFactory
}

object App {
  def fromDatabase(dbF: JdbcBackend#Database): Unit = ???

  def main(args: Array[String]) = {
    val backend = new JdbcBackend()
    fromDatabase(new backend.Database())
  }
}

This compiles in scala 2.13.16, but doesn't in any version of scala 3

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Thanks @prolativ for trying with other versions!

Given that my singe-file reproduction doesn't compile in any version of Scala 3, I don't see why the version split in two compilation units should compile. I agree with @jchyb's analysis: maybe this should never be valid and the compiler should inform us that an illegal type projection is used.

@Gedochao
Copy link
Contributor

Gedochao commented Apr 7, 2025

cc @odersky

@odersky odersky self-assigned this Apr 7, 2025
@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Running my single-file reproduction above with --explain:

sbt:scala3> scalac --explain playground.scala
[info] running (fork) dotty.tools.dotc.Main -d /Users/mbovel/dotty/compiler/../out/default-last-scalac-out.jar -classpath /Users/mbovel/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.16/scala-library-2.13.16.jar:/Users/mbovel/dotty/library/../out/bootstrap/scala3-library-bootstrapped/scala-3.7.1-RC1-bin-SNAPSHOT-nonbootstrapped/scala3-library_3-3.7.1-RC1-bin-SNAPSHOT.jar --explain playground.scala
-- [E007] Type Mismatch Error: playground.scala:13:15 --------------------------
13 |  fromDatabase(new backend.Database())
   |               ^^^^^^^^^^^^^^^^^^^^^^
   |               Found:    backend.DatabaseFactory
   |               Required: slickeffect.JdbcBackend#Database
   |----------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |
   | Tree: new backend.Database()
   | I tried to show that
   |   backend.DatabaseFactory
   | conforms to
   |   slickeffect.JdbcBackend#Database
   | but none of the attempts shown below succeeded:
   |
   |   ==> backend.DatabaseFactory  <:  slickeffect.JdbcBackend#Database  = false
   |
   | The tests were made under the empty constraint
    ----------------------------------------------------------------------------
1 error found
[error] Nonzero exit code returned from runner: 1

@odersky
Copy link
Contributor

odersky commented Apr 7, 2025

The example compiles OK if I replace Database by DatabaseFactory.

package slickeffect

class JdbcBackend {
  type Database = DatabaseFactory
  val Database: DatabaseFactory = ???
  class DatabaseFactory
}

object App {
  def fromDatabase(dbF: JdbcBackend#DatabaseFactory): Unit = ???

  def main(args: Array[String]) = {
    val backend = new JdbcBackend()
    fromDatabase(new backend.Database())
  }
}

So it looks like this should compile, since type aliases should not matter for subtyping checks.

@odersky
Copy link
Contributor

odersky commented Apr 8, 2025

So it looks like this should compile, since type aliases should not matter for subtyping checks.

In fact, on closer inspection, it should not compile.

  • type Database = DatabaseFactory is really type Database = this.DatabaseFactory since DatabaseFactory is an inner class.
  • When elaborating JdbcBackend#DatabaseFactory in asSeenFrom we need to replace the this but we have no path to replace it with. So we use an approximation: Nothing as the lower bound and JdbcBackend#Database as the upper bound.
  • Since we need the lower bound for the comparison in the test, the test fails.

This shows again how slippery and fragile type projections are. Maybe we should restrict them further, and only allow A#B if both A and B are classes? That would precisely cover the original reason why type projection was introduced (model Java's inner classes) without introducing any extra baggage.

@odersky
Copy link
Contributor

odersky commented Apr 8, 2025

Since the originally reported problem has the same underlying configuration as @mbovel's minimization, I guess it worked by accident before and rightly should not work anymore. Easy workaround: Don't use type aliases in type projection, expand to the original class instead.

@odersky odersky closed this as completed Apr 8, 2025
@Gedochao Gedochao closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2025
@Gedochao
Copy link
Contributor

Gedochao commented Apr 8, 2025

cc @kubukoz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:wontfix
Projects
None yet
Development

No branches or pull requests

6 participants