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

Problem with Traits not being Matchable #16408

Closed
bblfish opened this issue Nov 24, 2022 · 21 comments · Fixed by #17937
Closed

Problem with Traits not being Matchable #16408

bblfish opened this issue Nov 24, 2022 · 21 comments · Fixed by #17937

Comments

@bblfish
Copy link

bblfish commented Nov 24, 2022

A fully minimized version of code to duplicate the problem can be found in the comment below by Symon R..

Compiler version

scala 3.2.1 (older too) and nightly 3.3....

Minimized code

The code that does not compile is on this onepage branch of a repo specially for this Dotty issue. There is now only one file to look at: RDF.scala.

The code is duplicated in full here:

import scala.util.Try

trait RDF:
  rdf =>

  type R = rdf.type
  type Node <: Matchable
  type URI <: Node 

  given rops: ROps[R]
end RDF

object RDF:
  type Node[R <: RDF] = R match
    case GetNode[n] => Matchable //n & rNode[R]

  type URI[R <: RDF] <: Node[R] = R match
    case GetURI[u] => u & Node[R] 

  private type GetNode[N] = RDF { type Node = N }
  private type GetURI[U] = RDF { type URI = U }
end RDF

trait ROps[R <: RDF]:
  def mkUri(str: String): Try[RDF.URI[R]]
  def auth(uri: RDF.URI[R]): Try[String]

object TraitTypes:
  trait Node:
    def value: String

  trait Uri extends Node

  def mkUri(u: String): Uri =
    new Uri { def value = u }

object TraitRDF extends RDF:
  import TraitTypes as tz

  override opaque type Node <: Matchable = tz.Node
  override opaque type URI <: Node = tz.Uri

  given rops: ROps[R] with
    override def mkUri(str: String): Try[RDF.URI[R]] = Try(tz.mkUri(str))
    override def auth(uri: RDF.URI[R]): Try[String] =
      Try(java.net.URI.create(uri.value).getAuthority())

end TraitRDF

class Test[R <: RDF](using rops: ROps[R]):
  import rops.given
  lazy val uriT: Try[RDF.URI[R]] = rops.mkUri("https://bblfish.net/#i")
  lazy val x: String = "uri authority=" + uriT.map(u => rops.auth(u))

@main def run =
  val test = Test[TraitRDF.type]
  println(test.x)

Output

The compilation error can be seen in the github actions failing check here.
But I could not get the -explain flag to work with scala-cli.

Running it on the command line one gets the full error explantion. (After setting the env variables
below

export JAVA_HOME=/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home
export SCALA_HOME=/usr/local/scala/scala3-3.2.1

The following command

sh bin/clean
sh bin/compileScala
-- Error: scala/RDF.scala:44:8 -------------------------------------------------
44 |  given rops: ROps[R] with
   |        ^
   |object creation impossible, since def auth(uri: RDF.URI[R]): util.Try[String] in trait ROps is not defined
   |(Note that
   | parameter RDF.URI[R] in def auth(uri: RDF.URI[R]): util.Try[String] in trait ROps does not match
   | parameter TraitTypes.Uri & Matchable in override def auth(uri: TraitTypes.Uri & Matchable): util.Try[String] in object rops in object TraitRDF
   | )
-- [E038] Declaration Error: scala/RDF.scala:46:17 -----------------------------
46 |    override def auth(uri: RDF.URI[R]): Try[String] =
   |                 ^
   |   method auth has a different signature than the overridden declaration
   |----------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   | There must be a non-final field or method with the name auth and the
   | same parameter list in a super class of object rops to override it.
   |
   |   override def auth(uri: TraitTypes.Uri & Matchable): util.Try[String]
   |
   | The super classes of object rops contain the following members
   | named auth:
   |   def auth(uri: RDF.URI[TraitRDF.R]): util.Try[String]
    ----------------------------------------------------------------------------

But the compiler only has a problem when the URI is implemented in terms of a trait, not when it is implemented in terms of a class.

Expectation

The expectation is that it should work with traits as well as with classes.

A fix is to change the trait Uri to a class Uri as in the commit that lead to the onepage-works branch commit as shown in this minimal diff:

Screenshot 2022-12-19 at 21 04 36

The problem is that it is not always possible use classes only and not traits. A major RDF library RDF4J uses traits for its RDF model, such as IRI.

@bblfish bblfish added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Nov 24, 2022
@szymon-rd szymon-rd added stat:needs minimization Needs a self contained minimization stat:needs info labels Nov 24, 2022
@szymon-rd
Copy link
Contributor

szymon-rd commented Nov 24, 2022

Reproduced on nightly:
https://github.com/szymon-rd/DottyIssue16247/actions/runs/3540769029/jobs/5944219450

@bblfish Could you provide further minimization? It will be easier to address the problem if you provide a single scenario and make this issue self-contained. The best format would be the one that contains:

  • Description of the essence of the problem. What scenario does not work, why do you think it should, and what do you perceive as wrong?
  • Minimized scala-cli snippets. Preferably ones that should compile but do not. They should be the shortest possible example of the reasoning in the description.
  • Then, some additional information that may help in fixing the bug, for example, the fact that when you use classes instead of traits then it works.

The code that you provided is fairly complex. There is a high possibility that there is a way to show the issue with a shorter, less complex snippet, that would make it much easier for us to address this problem. Editing this issue to contain all the information will also help massively, as one can get pretty lost following the previous repositories, issues, and comments in them.

@bblfish
Copy link
Author

bblfish commented Nov 24, 2022

The code I posted currently stems from the use case I described in lib unification and match types so it has the advantage of describing a realistic situation.

But I agree. I'll try to remove stuff slowly and see how small I can get.
Doing that will take a bit of time though...

@szymon-rd
Copy link
Contributor

szymon-rd commented Nov 24, 2022

To be fair, it is not that valuable to represent a realistic situation. It's best to require as little context as possible. Representing a realistic situation often requires adding additional, unnecessary information. This is a bug report to the compiler, and it's best to stay on the same level of abstraction that the compiler is operating on - the syntactic and semantic structures. You can, of course, provide a rationale on why it is a bug by relating to a real-world problem - for example, argument that the compiler behaves confusingly in a common real-world case. But the problem itself should be kept as simple as possible, and having as narrow context as possible.

@bblfish
Copy link
Author

bblfish commented Nov 24, 2022

Ok, @szymon-rd I created a new branch minimization and was able to remove quite a lot of code, as shown by the history. commit bblfish/DottyIssue16247@4159ccd removed too much, and commit bblfish/DottyIssue16247@eaf445a put back the minmum for the problem to reappear, as one can see in the github actions.

The problem appears when I move from the functioning

trait RDF:
  rdf =>

  type R = rdf.type
  
  type Node <: Matchable
  type URI <: Node

  given rops: ROps[R]
end RDF

to the code that works when the types are implemented with classes but
not when they are implemented by reference to traits or java interfaces.

trait RDF:
  rdf =>

  type R = rdf.type
  type rNode <: Matchable
  type rURI <: rNode
  type Node <: rNode
  type URI <: Node & rURI

  given rops: ROps[R]
end RDF

I should have saved the intermediate point which also compiled which I think was

trait RDF:
  rdf =>

  type R = rdf.type
  type rURI <: Matchable
  type Node <: Matchable
  type URI <: Node & rURI

  given rops: ROps[R]
end RDF

For some reason that type of hierarchy works ok for classes but not when traits or interfaces are involved.
Note that in each of these cases the both URI and rURI as well as Node and rNode refer to the same
classes/interfaces. Eg for the class based implementation we have

  override opaque type rNode <: Matchable = cz.Node
  override opaque type rURI <: rNode = cz.Uri
  override opaque type Node <: rNode = cz.Node
  override opaque type URI <: Node & rURI = cz.Uri

and for the trait based implementation we have

 override opaque type rNode <: Matchable = rdfscala.TstNode
  override opaque type rURI <: rNode  = rdfscala.URI
  override opaque type Node <: rNode = rdfscala.TstNode
  override opaque type URI <: Node & rURI = rdfscala.URI

Why do I need that?

Well it allows me to use the concept that types are identified by the operations they support to make life of developers using our library easier. Jena and RDF4J don't really check carefully whether a node of type URI in a graph is a relative or absolute URLs, so we can easily allow both. But we would actually like to keep track of the difference.

  • Relative urls can take a base and be absolutized, whereas that operation makes no sense for absolute URIs. The inverse one does of relativizing a url to a base, say when POSTing content to a server in LDP, where the server then gives the resource a name.
  • We have allowed relative URLs in banana-rdf but that then was very annoying because I could never tell if I had a graph containing them or not. By making these types explicit we can track when we have graphs with relative urls and when we don't. Say if we read-in a graph with relative urls from the web, I know that there can be none if I fold over all the links in the graph and absolutize them. The underlying java objects are the same, but our knowledge has changed after the operation, and we can reflect that by moving from a graph with rURI to a graph containing only URIs.

@szymon-rd
Copy link
Contributor

Thank you for the additional information! This code is still really complex, and further minimization would be welcome. Making the code simpler and shorter will allow us to address this issue faster.

@dwijnand dwijnand removed the stat:needs triage Every issue needs to have an "area" and "itype" label label Nov 28, 2022
@bblfish
Copy link
Author

bblfish commented Dec 1, 2022

I have simplified the code again now here in the min2 branch. I removed the no-longer needed java interfaces, and I merged two files so as to reduce them down to 3. I also removed methods that were not playing a role in the tests.

https://github.com/bblfish/DottyIssue16247/blob/min2

Then I made sure that there was the least difference between the Class and Trait based code, which is now very small as can be seen by this opendiff snapshot.

Screenshot 2022-12-01 at 16 28 18

The code is run https://github.com/bblfish/DottyIssue16247/actions/runs/3594012563

@bblfish
Copy link
Author

bblfish commented Dec 2, 2022

I have now created a new branch min2-fix which sets out to fix the trait based code in two steps. The second commit to that branch involves just changing trait to abstract class which moves the build from broken to fixed. See the one line diff:

bblfish/DottyIssue16247@2b2e935

That should be a helpful way of looking at the problem as it narrows the change to one simple diff.

@szymon-rd
Copy link
Contributor

There is a lot of information in this post right now. Could you put a self-contained summary with the current status of minimisation with the links and all the required information to look into the problem in the issue itself? By that I mean editing it so it only contains the information that is up-to date and required to debug the issue, removing all the outdated information and other background info like history of the problem. It would be very helpful for us, and again thanks for your work minimizing and simplifying the snippets!

@bblfish
Copy link
Author

bblfish commented Dec 19, 2022

I have further simplified the code now, getting it down to one scala file, and removing the rURI and rNode types which were not part of the problem, as I had thought.

@szymon-rd szymon-rd self-assigned this Dec 20, 2022
@szymon-rd
Copy link
Contributor

szymon-rd commented Dec 20, 2022

Ok, so we minimized the code to this snippet:

object Helpers:

  type NodeFun[R] = Matchable // compiles without [R] parameter

  type URIFun[R] = R match
    case GetURI[u] => u & NodeFun[R] 
  private type GetURI[U] = RDF { type URI = U }
  
end Helpers

trait RDF:
  type URI 
end RDF

trait ROps[R <: RDF]:
  def auth(uri: Helpers.URIFun[R]): String
  

object TraitRDF extends RDF:
  override type URI = TraitTypes.UriImpl

  val z = new ROps[TraitRDF.type] {
    override def auth(uri: Helpers.URIFun[TraitRDF.type]): String = ???
  }
end TraitRDF

object TraitTypes:
  trait UriImpl // doesn't compile
  // class UriImpl // compiles

bblfish added a commit to bblfish/banana-rdf that referenced this issue Dec 21, 2022
@prolativ prolativ added area:typer area:match-types and removed stat:needs minimization Needs a self contained minimization labels Dec 27, 2022
@szymon-rd szymon-rd added this to the Future versions milestone Jan 16, 2023
@bblfish
Copy link
Author

bblfish commented Feb 10, 2023

Do you have any idea yet how complicated or deep a problem this bug has uncovered? Having this added to the "Future versions" milestone sounds very open-ended :-/

@sjrd
Copy link
Member

sjrd commented Apr 20, 2023

Minimized just a bit more, without requiring the GetURI extractor:

object Helpers:
  type NodeFun[R] = Matchable // compiles without [R] parameter

  type URIFun[R] = R match
    case RDF[u] => u & NodeFun[R]
end Helpers

trait RDF[URIParam]

trait ROps[R <: RDF[?]]:
  def auth(uri: Helpers.URIFun[R]): String

object TraitRDF extends RDF[TraitTypes.UriImpl]:
  val z = new ROps[TraitRDF.type] {
    override def auth(uri: Helpers.URIFun[TraitRDF.type]): String = ???
  }
end TraitRDF

object TraitTypes:
  trait UriImpl // doesn't compile
  //class UriImpl // compiles

Errors:

-- Error: tests/run/hello.scala:14:10 ------------------------------------------
14 |  val z = new ROps[TraitRDF.type] {
   |          ^
   |object creation impossible, since def auth(uri: Helpers.URIFun[R]): String in trait ROps is not defined 
   |(Note that
   | parameter Helpers.URIFun[R] in def auth(uri: Helpers.URIFun[R]): String in trait ROps does not match
   | parameter TraitTypes.UriImpl & Helpers.NodeFun[TraitRDF.type] in override def auth
   |  (uri: TraitTypes.UriImpl & Helpers.NodeFun[TraitRDF.type]): String in anonymous class Object with ROps[TraitRDF.type] {...} in object TraitRDF
   | )
-- [E038] Declaration Error: tests/run/hello.scala:15:17 -----------------------
15 |    override def auth(uri: Helpers.URIFun[TraitRDF.type]): String = ???
   |                 ^
   |   method auth has a different signature than the overridden declaration
   |
   | longer explanation available when compiling with `-explain`
2 errors found

For extra fun: if we replace u & NodeFun[R] by (u, NodeFun[R]) to avoid the intersection type, the code compiles.

@smarter Some weird erasure of intersection types thing perhaps?

@smarter
Copy link
Member

smarter commented May 3, 2023

If UriImpl is a trait, then UriImpl & Object erases to Object because intersection erasure prefers classes over traits, and only in case of ambiguity do we prefer subtypes over supertypes (always preferring classes is better for Java interoperability since it matches how Java intersection erasure works).

The problem is that match type reduction also performs some amount of simplification, so UriImpl & NodeFun[TraitRDF.type] gets simplified to UriImpl. I designed intersection type erasure with the assumption that we wouldn't be doing simplifications in method signatures since the simplification logic is implementation-defined, but it looks like match types violate that assumption.

@smarter
Copy link
Member

smarter commented May 3, 2023

Specifically, the culprit is the call to .simplified here:
https://github.com/lampepfl/dotty/blob/b8d2966e2f45e21f2800cd125ac72d80e5f2cb2a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L3152

@smarter
Copy link
Member

smarter commented May 3, 2023

The other piece of the puzzle here is that .simplified doesn't simplify intersections and unions when in Type mode: https://github.com/lampepfl/dotty/blob/b8d2966e2f45e21f2800cd125ac72d80e5f2cb2a/compiler/src/dotty/tools/dotc/core/TypeOps.scala#L158-L162

When we typecheck the auth override to figure out its type in Typer, Type is on and so we don't simplify the intersection, but when we check if its signature matches the overridden signature in RefChecks, Type is off and so when computing the overridden type we do simplify the intersection.

@smarter
Copy link
Member

smarter commented May 3, 2023

In other words, we might want to wrap TypeErasure#sigName in withMode(Mode.Type) to avoid simplifications that might change the signature, but we might still run into trouble if the match type reduction is cached since it doesn't look like the cache is invalidated based on modes, so it still seems like we should remove this .simplified call from the match type reduction logic.

@bblfish
Copy link
Author

bblfish commented May 15, 2023

It would be great if the fix could make it into the next release so that I can continue my work with banana-rdf. :-)
I have an EU milestone I need to do on that.

@smarter
Copy link
Member

smarter commented May 15, 2023

It's a bit awkward but you can probably work around the problem with some extra bounds and intersections with Object or Matchable, for example the following adapted from your original example compiles:

import scala.util.Try

trait RDF:
  rdf =>

  type R = rdf.type
  type Node <: Matchable
  type URI <: Node 

  given rops: ROps[R]
end RDF

object RDF:
  type Node[R <: RDF] <: Matchable = R match
    case GetNode[n] => Matchable //n & rNode[R]

  type URI[R <: RDF] <: Matchable & Node[R] = R match
    case GetURI[u] => u & Node[R] 

  private type GetNode[N] = RDF { type Node = N }
  private type GetURI[U] = RDF { type URI = U }
end RDF

trait ROps[R <: RDF]:
  def mkUri(str: String): Try[RDF.URI[R]]
  def auth(uri: RDF.URI[R] & Matchable): Try[String]

object TraitTypes:
  trait Node:
    def value: String

  trait Uri extends Node

  def mkUri(u: String): Uri =
    new Uri { def value = u }

object TraitRDF extends RDF:
  import TraitTypes as tz

  override opaque type Node <: Matchable = tz.Node
  override opaque type URI <: Node = tz.Uri

  given rops: ROps[R] with
    override def mkUri(str: String): Try[RDF.URI[R]] = Try(tz.mkUri(str))
    override def auth(uri: RDF.URI[R]): Try[String] =
      Try(java.net.URI.create(uri.value).getAuthority())

end TraitRDF

class Test[R <: RDF](using rops: ROps[R]):
  import rops.given
  lazy val uriT: Try[RDF.URI[R]] = rops.mkUri("https://bblfish.net/#i")
  lazy val x: String = "uri authority=" + uriT.map(u => rops.auth(u))

@main def run =
  val test = Test[TraitRDF.type]
  println(test.x)

@smarter

This comment was marked as outdated.

@bblfish
Copy link
Author

bblfish commented Jun 5, 2023

The pattern that is being used here is incredibly useful to tie different libraries togeteher efficiently in a unified interface. It is needed as a replacement to type projections discussed here
lampepfl/dotty-feature-requests#14

dwijnand added a commit to dwijnand/scala3 that referenced this issue Jun 7, 2023
Transcribing and paraphrasing from smarter's comment in
scala#16408 (comment) :

Type erasure assumes method signatures aren't simplified, since
simplification logic is implementation-defined.  For instance, some
intersection types can be simplified down, but intersection types and
their simplification can erase to different types - prefering classes
over traits, for instance (for Java interop, as it matches Java's
erasure).

Also note, simplify doesn't simplify intersections and unions in Type
mode.  But Match Types will cache their reduction without considering
the type mode as a cache input, thus the simplified reduction leaks even
when called in Type mode.

Perhaps we don't need to reach for simplified and can simplify normalise
down the reduced case (allowing for recursive match types to reduce
down).  Also the desire is to have both reducing cases normalize, rather
than just the first one.
@dwijnand dwijnand linked a pull request Jun 7, 2023 that will close this issue
dwijnand added a commit to dwijnand/scala3 that referenced this issue Jun 8, 2023
Transcribing and paraphrasing from smarter's comment in
scala#16408 (comment) :

Type erasure assumes method signatures aren't simplified, since
simplification logic is implementation-defined.  For instance, some
intersection types can be simplified down, but intersection types and
their simplification can erase to different types - prefering classes
over traits, for instance (for Java interop, as it matches Java's
erasure).

Also note, simplify doesn't simplify intersections and unions in Type
mode.  But Match Types will cache their reduction without considering
the type mode as a cache input, thus the simplified reduction leaks even
when called in Type mode.

So we call simplified in Mode.Type, in both cases (another desire), so
only that result is cached instead.

Using normalise doesn't work because, for example, that doesn't
normalise match types that are applied type args (e.g. args of Pair).
And not caching the result of those reductions means that they'll get
repeat over and over.
dwijnand added a commit to dwijnand/scala3 that referenced this issue Jun 20, 2023
There are a number of calls to ".simplified", which changes behaviour
based on Mode.Type.  It does _less_ under Mode.Type, so we
conservatively call it all under Mode.Type.

Transcribing and paraphrasing from smarter's comment in
scala#16408 (comment) :

Type erasure assumes method signatures aren't simplified, since
simplification logic is implementation-defined.  For instance, some
intersection types can be simplified down, but intersection types and
their simplification can erase to different types - prefering classes
over traits, for instance (for Java interop, as it matches Java's
erasure).

Also note, simplify doesn't simplify intersections and unions in Type
mode.  But Match Types will cache their reduction without considering
the type mode as a cache input, thus the simplified reduction leaks even
when called in Type mode.

So we call simplified in Mode.Type, in both cases (another desire), so
only that result is cached instead.

Using normalise doesn't work because, for example, that doesn't
normalise match types that are applied type args (e.g. args of Pair).
And not caching the result of those reductions means that they'll get
repeat over and over.
smarter added a commit that referenced this issue Jun 23, 2023
Transcribing and paraphrasing from @smarter's comment in
#16408 (comment) :

Type erasure assumes method signatures aren't simplified, since
simplification logic is implementation-defined.  For instance, some
intersection types can be simplified down, but intersection types and
their simplification can erase to different types - prefering classes
over traits, for instance (for Java interop, as it matches Java's
erasure).

Also note, simplify doesn't simplify intersections and unions in Type
mode.  But Match Types will cache their reduction without considering
the type mode as a cache input, thus the simplified reduction leaks even
when called in Type mode.

So we call simplified in Mode.Type, in both cases (another desire), so
only that result is cached instead.

Using normalise doesn't work because, for example, that doesn't
normalise match types that are applied type args (e.g. args of Pair).
And not caching the result of those reductions means that they'll get
repeat over and over.
@bblfish
Copy link
Author

bblfish commented Jun 24, 2023

It seems to be fixed now with the nightly version and I can compile the DottyIssue16247 onepage branch

scala-cli compile -j 20 -S 3.nightly scala

and similarly with the original java interface-based test now works with the nightly

scala-cli --scala 3.nightly scala/RDF_Java_Interface.scala scala/RDF.scala java/testorg/*

and the trait based code also compiles with the nightly

scala-cli --scala 3.nightly scala/RDF_Traits.scala scala/RDF_UsingScalaTrait.scala scala/RDF.scala

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

Successfully merging a pull request may close this issue.

7 participants