Skip to content

No warning/error when matching unrelated object types #9740

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

Closed
olofwalker opened this issue Sep 7, 2020 · 11 comments · Fixed by #11327
Closed

No warning/error when matching unrelated object types #9740

olofwalker opened this issue Sep 7, 2020 · 11 comments · Fixed by #11327

Comments

@olofwalker
Copy link

olofwalker commented Sep 7, 2020

Minimized code

Dotty 0.27-RC1

object Main:

  abstract class RecoveryCompleted
  case object RecoveryCompleted extends RecoveryCompleted
  
  abstract class TypedRecoveryCompleted 
  case object TypedRecoveryCompleted extends TypedRecoveryCompleted

  TypedRecoveryCompleted match {
    case RecoveryCompleted => println("Recovery completed")
    case TypedRecoveryCompleted => println("Typed recovery completed")
  }

Output

Nothing

Expectation

Something like:

pattern type is incompatible with expected type;
 found   : ammonite.$sess.cmd5.RecoveryCompleted.type
 required: ammonite.$sess.cmd7.TypedRecoveryCompleted.type
    case RecoveryCompleted => println("Recovery completed")
@liufengyun
Copy link
Contributor

This seems to be the correct behavior. See the following example:

class Error {
  abstract class RecoveryCompleted
  case object RecoveryCompleted extends RecoveryCompleted {
    override def equals(that: Any): Boolean = that == TypedRecoveryCompleted
  }

  abstract class TypedRecoveryCompleted
  case object TypedRecoveryCompleted extends TypedRecoveryCompleted


  TypedRecoveryCompleted match {
    case RecoveryCompleted => println("Recovery completed")
    case TypedRecoveryCompleted => println("Typed recovery completed")
  }
}

object Test {
  def main(args: Array[String]): Unit = new Error
}

The first branch matches.

@liufengyun
Copy link
Contributor

Related: #3200 scala/bug#1503

@olofwalker
Copy link
Author

olofwalker commented Sep 7, 2020

@liufengyun

This fails in 2.13, I just assumed this would also fail in Dotty.

pattern type is incompatible with expected type
[error]  found   : Error.this.RecoveryCompleted.type
[error]  required: Error.this.TypedRecoveryCompleted.type

The context for this is that I never got a RecoveryCompleted signal to fire in an Akka persistent actor, there are two of those classes, one for classic actors and one for typed, and I had imported and used the wrong one.

@liufengyun
Copy link
Contributor

liufengyun commented Sep 7, 2020

@olofwalker Thanks for clarifying the context. It's a difference between Scala 2 and Scala 3.

Maybe Scala 3 should be more conservative here to prevent such mistakes.

liufengyun added a commit to dotty-staging/dotty that referenced this issue Sep 7, 2020
@olofwalker
Copy link
Author

@liufengyun I think I would have been helped by a warning, its a bit like Scala 2, you unknowingly import an implicit, and all of a sudden things work differently.

@smarter
Copy link
Member

smarter commented Sep 7, 2020

We can't just copy what 2.13 is doing here as demonstrated by Fengyun's example in #9740 (comment), scalac wrongly emits an error even though this code is correct. We would have to check if equals in RecoveryCompleted is compiler-generated first

@liufengyun
Copy link
Contributor

We can't just copy what 2.13 is doing here as demonstrated by Fengyun's example in #9740 (comment), scalac wrongly emits an error even though this code is correct. We would have to check if equals in RecoveryCompleted is compiler-generated first

It is incomplete, but it seems sound/safe to reject the code. The additional expressiveness seems to be not useful in practice, and usually implies something goes wrong.

@smarter
Copy link
Member

smarter commented Sep 7, 2020

It's safe but it might be annoying to someone :). But I think if we only emit the warning if scrutinee.is(Case) && scrutineeEqualsMethod.is(Synthetic) then it's fine and catch the cases we're interested in.

@smarter
Copy link
Member

smarter commented Sep 7, 2020

Oops actually we want to check the symbol of the case, not the symbol of the scrutinee.

@liufengyun
Copy link
Contributor

After reflection, I think the correct thing to do is either (1) disallow all such cases; or (2) allow all such cases. Anything in the middle (like checking whether the member equals is overridden) only complicates the protocol.

Disallowing all such usage would align with Scala 2 and prevent accidental errors in practice. It's safe and sound -- incompleteness is fine, as static checks are all incomplete for good reasons.

Are there valid use cases that justify allowing such usage?

@dwijnand
Copy link
Member

dwijnand commented Feb 2, 2021

I agree. Can always make it more complicated if that proves too annoying - which hasn't appeared to be the case for Scala 2 users so far.

The argument around Fengyun's example to me seems to revolve around the fact that any scrutinee type (e.g. TypedRecoveryCompleted.type) is a subtype of Any and that any class can redefine its equals to be true for that singleton. But (1) that's unlawful for equals (though the type system can't rely on that) and (2) (more importantly) making that concession undermines the reachability checking/safety of all matches.

liufengyun added a commit to dotty-staging/dotty that referenced this issue Feb 5, 2021
We enforce that when pattern match on an object, the object should be
a subtype of the scrutinee type.

Reasons for doing so:

- such code patterns usually implies hidden errors in the code
- it's always safe/sound to reject the code

We could check whether `equals` is overridden in the object, but

- it complicates the protocol
- overriding `equals` of object is also a bad practice
- there is no sign that the slightly improved completeness is useful
liufengyun added a commit to dotty-staging/dotty that referenced this issue Feb 16, 2021
We enforce that when pattern match on an object, the object should be
a subtype of the scrutinee type.

Reasons for doing so:

- such code patterns usually implies hidden errors in the code
- it's always safe/sound to reject the code

We could check whether `equals` is overridden in the object, but

- it complicates the protocol
- overriding `equals` of object is also a bad practice
- there is no sign that the slightly improved completeness is useful
liufengyun added a commit that referenced this issue Feb 23, 2021
Fix #9740: harden type checking for pattern match on objects
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment