-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Context Bounds for Polymorphic Functions #21643
Context Bounds for Polymorphic Functions #21643
Conversation
@odersky Could you take a look and see if there is something fundamentally wrong with this approach? On a slightly different note: |
About aliases: type Cmp[X] = (x: X, y: X) => Ord[X] ?=> Boolean
type Comparer2 = [X: Ord] => Cmp[X] The tricky part is, say you have val f = [X; Ord] => (x: X, y: X) => Ord[X] ?=> true Does |
Also, we don't need to put this modularity anymore since SIP 64 got accepted. |
@odersky I'm not entirely sure that I understand your aliases example. e.g. [X: Ord] => (x: X, y: X) => Ord[X] ?=> Boolean
===>(desugar)
[X] => (x: X, y: X) => Ord[X] ?=> Ord[X] ?=> Boolean
===>(normalize)
[X] => (x: X, y: X) => Ord[X] ?=> Boolean |
Well, does |
It doesn't hold now. I'll try moving the expansion later, to be able to use some type info then. |
@odersky I rewrote the whole implementation to first mark |
I guess this should be merged in 3.7 now? @KacperFKorban @WojciechMazur |
+1, it's too late for this to be included in 3.6 (unless it's behind an experimental flag) |
@bishabosha @KacperFKorban Actually, it seems we will be backporting this change to 3.6.2 (which, despite appearances, is technically considered the first stable version in the 3.6.x line). |
I don't know if it was considered, but you could skip to 3.7.0 and declare all 3.6.x as invalid. |
It was considered, but we decided to keep 3.6.x instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is good, with scope for some better comments. We need to refine the logic where evidence parameters are added.
@@ -508,7 +514,24 @@ object desugar { | |||
case Nil => | |||
params :: Nil | |||
|
|||
cpy.DefDef(meth)(paramss = recur(meth.paramss)) | |||
def pushDownEvidenceParams(tree: Tree): Tree = tree match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a doc comment explaining what it does.
/** Push down the deferred evidence parameters up until the result type is not | ||
* a method type, poly type or a function type | ||
*/ | ||
private def pushDownDeferredEvidenceParams(tpe: Type, params: List[untpd.ValDef], span: Span)(using Context): Type = tpe.dealias match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is too coarse. If the evidence parameter is named, a parameter could already refer in its type to it. So the clause has to go just in front of this parameter, as is explained in Section 3 of SIP 64 and implemented in Desugar.addEvidenceParams
. We should see how we can re-use this logic.
Will nested polymorphic functions work? like |
Do we have any estimation of when this PR could be finalized? |
I'll try to address the review comments today/tomorrow. |
ping |
…ns to reuse some of the existing context bound expansion
…ction type definitions
…plementation consistent with addEvidenceParams
72d4d35
to
24e3fa0
Compare
…e no context bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small questions. I think we should have at least a simple run
test to ensure the context parameters are working. Otherwise, LGTM.
@odersky I changed the implementation to reuse the logic from
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, nice factoring of the common logic. I have only minor change requests.
Backports #21643 to the 3.6.2. PR submitted by the release tooling.
Implement the
#6
point form SIP-64 i.e.6. Context Bounds for Polymorphic Functions
Currently, context bounds can be used in methods, but not in function types or function literals. It would be nice propose to drop this irregularity and allow context bounds also in these places. Example:
The expansion of such context bounds is analogous to the expansion in method types, except that instead of adding a using clause in a method, we insert a context function type.
For instance, the
type
andval
definitions above would expand toThe expansion of using clauses does look inside alias types. For instance,
here is a variation of the previous example that uses a parameterized type alias:
The expansion of the right hand side of
Comparer2
expands theCmp[X]
aliasand then inserts the context function at the same place as what's done for
Comparer
.Currently missing: