Remove class tags from Tasty reflect interface #4904
Conversation
nicolasstucki
force-pushed
the
fix-type-unsoundness-in-tasty-reflect
branch
from
0984706 to
2862af6
Compare
|
Another CLA failure :( |
nicolasstucki
force-pushed
the
fix-type-unsoundness-in-tasty-reflect
branch
2 times, most recently
from
ca9bd9e to
a2a8d3f
Compare
There was a problem hiding this comment.
Most of this LGTM except for the dropped TODOs and for #4917 — the IsDefinition bug belongs here probably.
| object Id extends IdExtractor { | ||
| def unapply(x: Id): Option[String] = x match { | ||
| case x: untpd.Ident => Some(x.name.toString) // TODO how to make sure it is not a Ident or TypeIdent? Check x.tpe? |
There was a problem hiding this comment.
Why drop this TODO? That's not covered by the commit description.
Should this unapply return Some[String], or is this an accident? A comment clarifying which would help.
| object SimpleSelector extends SimpleSelectorExtractor { | ||
| def unapply(x: ImportSelector)(implicit ctx: Context): Option[Id] = x match { | ||
| case x: untpd.Ident => Some(x) // TODO make sure it will not match other idents | ||
| case x: untpd.Ident => Some(x) |
There was a problem hiding this comment.
Ditto, why drop that TODO?
| def unapply(tree: Tree)(implicit ctx: Context): Option[Definition] = tree match { | ||
| case tree: tpd.MemberDef => Some(tree) | ||
| case _ => None | ||
| } |
There was a problem hiding this comment.
By analogy with IsPackageClause, it seems we should have type Definition = tpd.MemberDef. Trying that reveals that definitionFromSym needs type Definition = tpd.MemberDef | PackageDef instead (which we can't use yet, pending bootstrap), but that means we're missing the PackageDef case here? #4917 changes that, but has no test.
There was a problem hiding this comment.
I modified PackageDef to PackageDefinition.
There was a problem hiding this comment.
That's fine, but we have type PackageDef = PackageDefinition. You're thinking of type PackageClause = tpd.PackageDef, but that's unrelated. Sadly I don't see an easy refactoring to avoid this confusion...
| def packageClauseClassTag: ClassTag[PackageClause] = implicitly[ClassTag[PackageClause]] | ||
| object IsPackageClause extends IsPackageClauseExtractor { | ||
| def unapply(tree: Tree)(implicit ctx: Context): Option[PackageClause] = tree match { | ||
| case x: tpd.PackageDef @unchecked => Some(x) |
There was a problem hiding this comment.
@unchecked is unneeded because we're matching on a tpd.Tree. Dropped in #4917, together with existing ones and ones added below.
Before, there was a super type of all abstract types in scala.Tasty. This made it possible to match some ident with the wrong type of extractor.
We found that when the implementation of those class tags uses the same class tags, type matches are unsound. Instead, we replace the more precise scrutinee by equivalent extractors. In the future, we intend to language support for abstract type pattern matching where the scrutinee can get a more precise type.
nicolasstucki
force-pushed
the
fix-type-unsoundness-in-tasty-reflect
branch
from
a2a8d3f to
a1e49b5
Compare
We found that when the implementation of those class tags
uses the same class tags, type matches are unsound.
Instead, we replace the more precise scrutinee by equivalent
extractors. In the future, we intend to language support
for abstract type pattern matching where the scrutinee
can get a more precise type.