Skip to content

Fix #7554: Add TypeTest for sound pattern type test #7555

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

Merged
merged 3 commits into from
Nov 12, 2020

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Nov 14, 2019

Comparison with other kinds of type representations https://gist.github.com/nicolasstucki/ae3eaae553f594066bf9ad0e817f9a63

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I strongly maintain that we should use ClassTag for this and not TypeTest. Let's fix/generalize broken abstractions instead of inventing new ones. That's the only way to keep complexity down.

@sjrd
Copy link
Member

sjrd commented Nov 23, 2019

And I strongly maintain that ClassTags are not broken, and that it was type tests bolted on them that were broken since the beginning. Let's not fix what ain't broken, and let's put in two unrelated interfaces the two completely unrelated capabilities, namely that of creating Array[A]s and that of testing x.isInstanceOf[A]. Maintaining unrelated responsibilities in different interfaces is the only way to keep complexity down.

@nicolasstucki nicolasstucki marked this pull request as ready for review December 4, 2019 18:27
@nicolasstucki nicolasstucki dismissed odersky’s stale review January 30, 2020 10:32

As @sjrd stated the issue is not that that ClassTags are broken but only the type test functionality that was added. This redefines the type tests functionality in a separate TypeTest class that has more a generalized API and stricter contract that fixes the shortcoming mentioned in #7554 and https://infoscience.epfl.ch/record/261291?ln=en .

@nicolasstucki
Copy link
Contributor Author

We still need this to make the tasty.Reflection interface sound.

@dwijnand
Copy link
Member

Is there a legal open-access version of https://infoscience.epfl.ch/record/261291?

@nicolasstucki
Copy link
Contributor Author

It looks like the paper can be freely downloaded from https://dl.acm.org/doi/10.1145/3241653.3241658

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I am still deeply opposed to introduce yet another form of type tag. We have too many of those already! So, I am sorry, that's a no go. At least not until we have tried out and tested all the alternatives.

Let's try to make a subtype of ClassTag that tests for prefixes. If that has a chance of working, it's much preferable to any alternative. I know some of you disagree on this, but I'll put my foot down on this one.

@odersky odersky assigned nicolasstucki and unassigned odersky May 18, 2020
@nicolasstucki
Copy link
Contributor Author

In #8993 I found that providing ClassTags were the type test is sound can lead unsoundess when using those ClassTags in arrays.

@nicolasstucki
Copy link
Contributor Author

@milessabin
Copy link
Contributor

milessabin commented Sep 26, 2020

What do people think about this alternative?

If it preserves (or improves the precision of) the semantics of shapeless's Typeable then 👍

shapeless has a gadget for using its Typeable in pattern matches, but its ergonomics on Scala 2 aren't great,

@Test
def testTypeCase: Unit = {
  import HList.ListCompat._

  def typeCase[T: Typeable](t: Any): Option[T] = {
    val T = TypeCase[T]
    val `List[T]` = TypeCase[List[T]]
    val `(String, T)` = TypeCase[(String, T)]
    val `List[(String, T)]` = TypeCase[List[(String, T)]]

    t match {
      case T(t) => Some(t)
      case `List[T]`(lt) => lt.headOption
      case `(String, T)`(s, t) => typed[String](s) ; Some(t)
      case `List[(String, T)]`((s, t) :: _) => typed[String](s); Some(t)
      case `List[(String, T)]`(lst) => assertTrue(lst.isEmpty) ; None
      case _ => None
    }
  }

  assertEquals(Some(23), typeCase[Int](23: Any))
  assertEquals(None, typeCase[String](23: Any))
  assertEquals(Some(23), typeCase[Int](List(23): Any))
  assertEquals(None, typeCase[String](List(23): Any))
  assertEquals(Some(23), typeCase[Int](("foo", 23): Any))
  assertEquals(None, typeCase[String](("foo", 23): Any))
  assertEquals(Some(23), typeCase[Int](List(("foo", 23)): Any))
  assertEquals(None, typeCase[String](List(("foo", 23)): Any))
}

It would be great if Scala 3 supported pattern syntax similar to the above without having to resort to backtick puns.

`TypeTest` is a replacemnt for the same functionallity performed by the `ClassTag.unaplly`.
Using `ClassTag` instances happend to be unsound.
`TypeTest` fixes that unsoundess and adds extra flexibility with the `S` type.
`ClassTag` type tests will still be supported but a warining will be emitted after 3.0.
@nicolasstucki
Copy link
Contributor Author

Rebased

@nicolasstucki nicolasstucki modified the milestones: 3.0.0, 3.0.0-M1, 3.0.0-M2 Nov 4, 2020
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Code looks good, it's just the doc page that needs some fixes.

@nicolasstucki

This comment has been minimized.

@nicolasstucki nicolasstucki merged commit 0134f2c into scala:master Nov 12, 2020
@nicolasstucki nicolasstucki deleted the fix-#7554 branch November 12, 2020 17:11
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.

Unsound use of ClassTag.unapply
8 participants