Skip to content

Remove Reflection.LambdaType #9861

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

Conversation

nicolasstucki
Copy link
Contributor

This can be encoded with intersection types in the few places where it is relevant

Based on #9818

@@ -1179,7 +1179,7 @@ class SourceCodePrinter[R <: Reflection & Singleton](val tasty: R)(syntaxHighlig
printType(body)

case ParamRef(lambda, idx) =>
lambda match {
(lambda: @unchecked) match {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liufengyun I was getting the following exhaustivity fatal warning where lambda: MethodType | PolyType | TypeLambda. Same if did the cases with case lambda: MethodType => .... It might be related to having a class tag preforming the type test.

[error] 1182 |        lambda match {
[error]      |        ^^^^^^
[error]      |match may not be exhaustive.
[error]      |
[error]      |It would fail on pattern case: _: MethodType, _: PolyType, _: TypeLambda

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's unknown that the extractors are irrefutable for the corresponding type MethodType or PolyType .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we make ClassTag.unapply (and later TypeTest.unapply) extractors irrefutable? It feels like they should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

ClassTag.unapply is defined as follows:

  def unapply(x: Any): Option[T] =
    if (runtimeClass.isInstance(x)) Some(x.asInstanceOf[T])
    else None

It's not irrefutable in general. Semantically, we may assume it's irrefutable if we know x takes the type T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the contract of the method. If x does not take the type T then implementation is broken and we cannot reason about the patterns correctly anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the following supposed to be irrefutable?

lambda match
  case lambda: MethodType =>
  case lambda: PolyType =>
  case lambda: TypeLambda =>

}

given ParamRefMethods as ParamRefMethods = ParamRefMethodsImpl
protected val ParamRefMethodsImpl: ParamRefMethods

trait ParamRefMethods:
extension (self: ParamRef):
def binder: LambdaType
def binder: MethodType | PolyType | TypeLambda
Copy link
Contributor Author

@nicolasstucki nicolasstucki Sep 24, 2020

Choose a reason for hiding this comment

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

We could also just use

      def binder: Type

In theory, this should only help with pattern matching exhaustivity which currently fails. See other comments.

This can be encoded with intersection types in the few places where it is relevant
@nicolasstucki nicolasstucki force-pushed the remove-reflection-lambdatype branch from b962805 to 4a8a1a7 Compare September 24, 2020 14:14
@@ -1966,8 +1967,6 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext:
end extension
end RecursiveTypeMethodsImpl

type LambdaType = dotc.core.Types.LambdaType
Copy link
Member

Choose a reason for hiding this comment

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

What do we gain by removing this? And if a union is preferable but needs to be used in multiple context, maybe we could have type LambdaType = MethodType | PolyType | TypeLambda ?

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.

3 participants