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

Opaque type leakage in Conversion typeclass definition #14070

Closed
nbenns opened this issue Dec 7, 2021 · 7 comments
Closed

Opaque type leakage in Conversion typeclass definition #14070

nbenns opened this issue Dec 7, 2021 · 7 comments

Comments

@nbenns
Copy link

nbenns commented Dec 7, 2021

Compiler version

3.1.0

Minimized code

FirstName.scala

opaque type FirstName = String

object FirstName {
  def apply(s: String): FirstName = s
}

FullName.scala

opaque type FullName = String
object FullName {
  def apply(s: String): FullName = s
  
  given Conversion[FullName, FirstName] with {
    def apply(fullName: FullName): FirstName = fullName.split(" ")(0)
  }
}

Main.scala

object Main extends App {
  def printFirstName(f: FirstName): Unit = {
    println(f)
  }

  val fn = FullName("Johnny Test")

  printFirstName(fn)
}

Output

Inifinite Loop / Hang

Expectation

Should not compile as the Conversion given should not know that String =:= FirstName

@prolativ
Copy link
Contributor

prolativ commented Dec 8, 2021

Actually there's no leak of opaque types here. This works according to the specification of opaques. Because given Conversion[FullName, FirstName] is defined inside the scope where FullName is defined, FullName is not considered opaque so it's seen as String. This in turn makes Conversion[FullName, FirstName] work as Conversion[String, FirstName]. Because of that there's an infinite loop in

def apply(fullName: FullName): FirstName = fullName.split(" ")(0)

because fullName.split(" ")(0) is a String but the expected type is FirstName.
The fix would be

def apply(fullName: FullName): FirstName = FirstName(fullName.split(" ")(0))

So this issue is basically a duplicate of #10947 and #13542. However it looks like usages of Conversion were somehow missed in #13589, which was supposed to introduce warnings for such loops.

@nbenns
Copy link
Author

nbenns commented Dec 8, 2021

So I don't agree with this, FirstName is the expected return type, and inside the FullName object it should know nothing about FirstName being a String, as we aren't in the scope where it should understand that. This should fail with a compilation error that FirstName is not a String in the same way as it would if it was defined outside the object. That is the expected and documented behavior according to the Scala 3 guide:

Top-level Opaque Types

An opaque type alias on the top-level is transparent in all other top-level definitions in the sourcefile where it appears, but is opaque in nested objects and classes and in all other source files.

From a user perspective, its violating the type signature and should not compile in the first place.

@prolativ
Copy link
Contributor

prolativ commented Dec 8, 2021

The compiler doesn't claim that FirstName =:= String inside FullName. That's why it tries to apply the implicit conversion (which calls itself recursively).

@nbenns
Copy link
Author

nbenns commented Dec 8, 2021

Oh I see, that makes sense. So its basically wrapping the conversion around it over and over (effectively). I'm surprised it even compiles in that case. That's why the only issue is with the special Conversion typeclass. Thanks for that explanation.

@odersky
Copy link
Contributor

odersky commented Dec 8, 2021

I agree with the analysis and I think we can close the issue.

@odersky odersky closed this as completed Dec 8, 2021
@som-snytt
Copy link
Contributor

I looked at this last night just before bed, thought it was normal scoping issue, put the files into separate packages etc, but it still compiled without error.

I will look again in clear light of day, in case I am ever fully awake today, but my takeaway is that I still don't have a quick way of asking a tool, why does this conversion apply and why isn't that line erroneous?

Maybe such a tool should be a plugin available with the REPL, in the same vein that you don't want the compiler to perform every lint, you don't want it to perform every mode of instrumentation.

@nbenns
Copy link
Author

nbenns commented Dec 8, 2021

Just a quick logical thought, but is this just due to the fact that conversions shouldn't be applied to its own internal structure? Recursive definitions actually seem to be the problem here, is there any situation where they are actually safe? I suppose you can never know if by using another conversion (not the one you are defining) if it will be mutually recursive or not with the current one, so its a loosing game.

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

No branches or pull requests

4 participants