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

Remove class tags from Tasty reflect interface #4904

Conversation

Projects
None yet
2 participants
@nicolasstucki
Copy link
Contributor

commented Aug 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Another CLA failure :(

@nicolasstucki nicolasstucki force-pushed the dotty-staging:fix-type-unsoundness-in-tasty-reflect branch 2 times, most recently from ca9bd9e to a2a8d3f Aug 8, 2018

@Blaisorblade
Copy link
Contributor

left a 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?

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 9, 2018

Contributor

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)

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 9, 2018

Contributor

Ditto, why drop that TODO?

def unapply(tree: Tree)(implicit ctx: Context): Option[Definition] = tree match {
case tree: tpd.MemberDef => Some(tree)
case _ => None
}

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 9, 2018

Contributor

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.

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Aug 9, 2018

Author Contributor

I modified PackageDef to PackageDefinition.

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 9, 2018

Contributor

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)

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Aug 9, 2018

Contributor

@unchecked is unneeded because we're matching on a tpd.Tree. Dropped in #4917, together with existing ones and ones added below.

nicolasstucki added some commits Aug 7, 2018

Remove class tags from Tasty reflect interface
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.
Remove outadted TODOs
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.

@nicolasstucki nicolasstucki force-pushed the dotty-staging:fix-type-unsoundness-in-tasty-reflect branch from a2a8d3f to a1e49b5 Aug 9, 2018

@Blaisorblade
Copy link
Contributor

left a comment

LGTM now.

@nicolasstucki nicolasstucki merged commit dbc8d80 into lampepfl:master Aug 9, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@nicolasstucki nicolasstucki deleted the dotty-staging:fix-type-unsoundness-in-tasty-reflect branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.