Skip to content

Test interaction between opaque and match #5436

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 1 commit into from
Nov 14, 2018

Conversation

abgruszecki
Copy link
Contributor

See #5300 (comment).

@Blaisorblade can you please merge this?

// both of the patterns need to be well-typed here,
// since Pos is potentially equal to any other type
case IntExpr(_) => Pos.mkPos(1)
case StrExpr(_) => ???

Choose a reason for hiding this comment

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

it's a shame this is not seen as dead code. Expr[T] is sealed, and there are only three valid values for T: Pos, Int and String, each of which is fully determined by the subclass.

Would be nice if case PosExpr(p) => p was seen as a total match here.

Copy link
Contributor

@LPTK LPTK Nov 13, 2018

Choose a reason for hiding this comment

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

You can have that (seeing case StrExpr(_) as dead) by moving the match to the companion of the opaque type. You should not have that otherwise, as it would be making the abstraction leak, which is very bad.

Also, case PosExpr(p) is not "total" as any IntExpr(_) can also be given type Expr[Pos].

Choose a reason for hiding this comment

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

right, I neglected the existence of coerce here, which can translate F[Int] to F[Pos]. I see your point: for an opaque type, we have to inspect and understand all possible functions like this in the companion, and it may be pretty non-trivial to rule out certain branches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @LPTK.
@johnynek There are two different problems — checking whether StringExpr is dead and whether IntExpr is dead. Checking those scenarios would break guarantees you expect from a type system. For both, it'd only be fine in whole-program static analysis tools, but you have different expectations. This is a tradeoff currently under discussion.

  • As @LPTK said, it's easy to figure out that StrExpr is dead once you know that Pos is implemented as Int — ignoring eval(StrExpr.asInstanceOf[Expr[Pos]) (which we should). But we shall not do it to avoid abstraction leaks. To be more precise on the guarantee: If client code compiles and you change Pos to be implemented as String, the client code should keep compiling.
  • It's hard to figure out whether IntExpr is dead, by checking whether coerce-like functions exist. It might be possible by static analysis of the companion object, but again you change warnings in client code depending on implementation details that (it seems) should be hidden (and are not visible in types). That breaks separate compilation/separate typechecking. Type systems usually don't do that, because if they did it'd be much harder to keep a library source-compatible. Especially with -Xfatal-warnings.

Copy link
Contributor

@Blaisorblade Blaisorblade left a comment

Choose a reason for hiding this comment

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

LGTM. I am a bit surprised this already works today, but that's good.
@odersky seemed to have concerns, but if they materialize we can change the test later (we do it often): it's still important not to change this behavior silently.

@Blaisorblade Blaisorblade merged commit 7a86d5b into scala:master Nov 14, 2018
@Blaisorblade Blaisorblade deleted the opaques-patmat branch November 14, 2018 18:13
@abgruszecki
Copy link
Contributor Author

For the record: I understood the concerns to be about when to expose the opaque alias as a true alias, specifically that exposing it too late could prevent some useful optimisations from applying. This test only verifies that it is possible to write an actual total pattern match against a GADT with an opaque alias as a parameter, which depends on the opaque alias not being exposed when typing the match - something which I'd be very surprised if we couldn't rely on.

@Blaisorblade
Copy link
Contributor

This test only verifies that it is possible to write an actual total pattern match against a GADT with an opaque alias as a parameter, which depends on the opaque alias not being exposed when typing the match - something which I'd be very surprised if we couldn't rely on.

What surprised me is that we get no redundancy warnings from the pattern matcher: I thought those warnings are emitted after the opaque type is dealiased.

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.

4 participants