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

SI-9394 Fix pattern infererence with class tparams in pt #4625

Closed
wants to merge 2 commits into from

Conversation

retronym
Copy link
Member

Constructor patterns must be compatible with the expected type of
the pattern (ie, the scrutinee type). The spec doesn't cover the
whole story here:

http://www.scala-lang.org/files/archive/spec/2.11/08-pattern-matching.html#constructor-patterns

If the case class is monomorphic, then it must conform to the
expected type of the pattern. [...]
If the case class is polymorphic, then its type parameters are
instantiated so that the instantiation of c conforms to the expected
type of the pattern

The implementation actually treats monomorphic case classes, like
those in the test case, according the rules for polymorphic case
classes, albeit with an empty type parameter list. As such, we need
to consider another part of the spec that elaborates on type parameter
inference within patterns, and the subsequent static check for
the pattern conforming to the expected type:

http://www.scala-lang.org/files/archive/spec/2.11/08-pattern-matching.html#type-parameter-inference-in-patterns

Otherwise, if T can not be made to conform to pt by instantiating
its type variables, one determines all type variables in pt which
are defined as type parameters of a method enclosing the pattern
...
let C2 be the weakest set of subtype constraints over the type
variables a1,…,an and b1,…,bm such that C0∧C′0∧C2 implies that
it is possible to construct a type T′ which conforms to both
T and pt. It is a static error if there is no satisfiable set of
constraints C2 with this property.

This "otherwise" corresponds to inferForApproxPt within:

https://github.com/scala/scala/blob/v2.11.6/src/compiler/scala/tools/nsc/typechecker/Infer.scala#L1063-L1085

This substitutes free type parameters within the expected type
with wildcard types. However, class type parameters were being
excluded from this due to the line of code that this commit removes.

I traced the owner.isTerm condition back to f9e5afd.
I don't understand the intent; and the tests are still passing.

I have also updated the spec to replace the too-narrow description
of pattern inference in the "constructor patterns" section with a link
to the more up-to-date description in the "pattern inference"
section, and to explain that mono- and polymorphic case classes
are treated uniformly when it comes to checking conformance to the
expected type.

Constructor patterns must be compatible with the expected type of
the pattern (ie, the scrutinee type). The spec doesn't cover the
whole story here:

http://www.scala-lang.org/files/archive/spec/2.11/08-pattern-matching.html#constructor-patterns

> If the case class is monomorphic, then it must conform to the
> expected type of the pattern. [...]
> If the case class is polymorphic, then its type parameters are
> instantiated so that the instantiation of c conforms to the expected
> type of the pattern

The implementation actually treats monomorphic case classes, like
those in the test case, according the rules for polymorphic case
classes, albeit with an empty type parameter list. As such, we need
to consider another part of the spec that elaborates on type parameter
inference within patterns, and the subsequent static check for
the pattern conforming to the expected type:

http://www.scala-lang.org/files/archive/spec/2.11/08-pattern-matching.html#type-parameter-inference-in-patterns

> Otherwise, if T can not be made to conform to pt by instantiating
> its type variables, one determines all type variables in pt which
> are defined as type parameters of a method enclosing the pattern
> ...
> let C2 be the weakest set of subtype constraints over the type
> variables a1,…,an and b1,…,bm such that C0∧C′0∧C2 implies that
> it is possible to construct a type T′ which conforms to both
> T and pt. It is a static error if there is no satisfiable set of
> constraints C2 with this property.

This "otherwise" corresponds to `inferForApproxPt` within:

https://github.com/scala/scala/blob/v2.11.6/src/compiler/scala/tools/nsc/typechecker/Infer.scala#L1063-L1085

This substitutes free type parameters within the expected type
with wildcard types. However, class type parameters were being
excluded from this due to the line of code that this commit removes.

I traced the `owner.isTerm` condition back to f9e5afd.
I don't understand the intent; and the tests are still passing.

I have also updated the spec to replace the too-narrow description
of pattern inference in the "constructor patterns" section with a link
to the more up-to-date description in the "pattern inference"
section, and to explain that mono- and polymorphic case classes
are treated uniformly when it comes to checking conformance to the
expected type.
@scala-jenkins scala-jenkins added this to the 2.12.0-M3 milestone Jul 13, 2015
  - Don't collect the free types from the pattern expected type
    until we need them
  - Don't run the `TreeTypeSubstituter` over the pattern tree
    for monomorphic case classes.
@retronym
Copy link
Member Author

Review by @odersky / @adriaanm

@adriaanm
Copy link
Contributor

The owner.isTerm check for type symbols is likely related to GADT inference, as it only affects type params owned by a term (the method). These type params are instantiated based on the case we are type checking.

@adriaanm
Copy link
Contributor

(I'll take another look with less sleepy eyes tomorrow.)

@lrytz lrytz modified the milestones: 2.12.0-M3, 2.12.0-M4 Oct 5, 2015
@adriaanm
Copy link
Contributor

I propose we close this until we have time to dig deeper and convince ourselves this moves the soundness needle in the right direction. This are of the type checker definitely needs improvement, but I think it needs a concerted effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants