Skip to content

Fix #5481: Follow opaque aliases in prototypes of functions #5485

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 2 commits into from
Nov 25, 2018

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 20, 2018

If the prototype of a function value is a synthetic opaque type alias, assume
the alias type. If the alias type is a function type, this allows one to infer
the parameter types of the function value.

@@ -728,13 +728,12 @@ class Typer extends Namer
case _: WildcardType => untpd.TypeTree()
case _ => untpd.TypeTree(tp)
}
pt.stripTypeVar match {
case _ if defn.isNonDepFunctionType(pt) =>
pt.dealias.followSyntheticOpaque match {
Copy link
Member

@smarter smarter Nov 20, 2018

Choose a reason for hiding this comment

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

This seems fishy to me, outside of the companion of the opaque type, we shouldn't be doing this. And inside the companion, it should not be needed (since we can just dealias). The real issue I think is that isNonDepFunctionType is overly strict in what it considers a function type, such that an intersection of a function type and something else is not considered a function type. If we relax this, then I don't think the followSyntheticOpaque is needed. (This is closely related to #5473 where the issue is that the definition of implicit function type is too restrictive).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dealiasing does not work inside the companion object, since for an opaque type

opaque type T = A

the binding of T.T has the form

type T >: A | outer.T <: A & outer.T

That's why followSyntheticOpaque is needed; it uncovers the alias A when inside the companion object.

Copy link
Member

Choose a reason for hiding this comment

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

But If I look at the base types of T in the companion object, I should see Function1[A, Boolean], because base types follow upper bounds, so if isNonDepFunctionType used isRef or something like that, we wouldn't need the special case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have to be derivesFrom but that would break other things. The code here is quite finely balanced out, and I don't want to upset it since this might take a long time and the result is probably the same.

Copy link
Member

Choose a reason for hiding this comment

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

I'm mostly mentioning this because #5473 could be solved the same way (using derivesFrom) and if we don't do that then more special cases are going to break (e.g. an implicit function type behind an opaque type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose to treat #5473 separately. I feel that's a much bigger can of worms than it looks at first.

…functions

If the prototype of a function value is a synthetic opaque type alias, assume
the alias type. If the alias type is a function type, this allows one to infer
the parameter types of the function value.
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

@odersky odersky merged commit 92bbb6e into scala:master Nov 25, 2018
@allanrenucci allanrenucci deleted the fix-#5481 branch November 26, 2018 08:12
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