Skip to content

Add prototype conversion with dependent functions support #8523

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

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Mar 12, 2020

Instead of writing a Conversion as

Conversion[T, U]

we would use

Conversion[T => U]

which also allows dependent conversion

Conversion[(x:T) => x.U]

Conversion (ConversionFunction in the test) would be an opaque type

opaque type Conversion[+F <: Nothing => Any] = F

This alternative definition of Conversion would fix the issue stated in #7412.

@nicolasstucki nicolasstucki force-pushed the add-prototye-conversion-with-dependent-functions-suport branch from c5399fd to ac4d72b Compare March 12, 2020 06:44
@nicolasstucki nicolasstucki force-pushed the add-prototye-conversion-with-dependent-functions-suport branch from ac4d72b to 075b6dd Compare March 12, 2020 08:39
@nicolasstucki nicolasstucki marked this pull request as ready for review March 12, 2020 08:48
@nicolasstucki nicolasstucki force-pushed the add-prototye-conversion-with-dependent-functions-suport branch from 075b6dd to fa76c6f Compare March 12, 2020 09:13
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
Copy link
Contributor

odersky commented Mar 12, 2020

I am not convinced this will solve the problem. There is some very funky code in the compiler to support dependent implicit conversions. The problem is that eligible implicits are pre-computed and cached. So,
if we have an implicit conversion of type x: T => x.M, say, it still has to fit into the framework that it is a conversion between two types A and B, because that's what we search the cache with. A = T, but what is B? We can't say B = Any since then the conversion would not fit anything. Instead we say it's B = <?>, where <?> is the wildcard type.

Now this is already quite dubious because <?> is not really a type but a prototype and we should not populate our caches with this. It's also potentially very inefficient. What will happen now is that the conversion has to be tried for every source type matching T since it might also match the target type. Depending what your implicit search patterns are, this could slow down compilation a lot.

So, in summary, I would actually be rather glad if we could drop support for dependent conversions once implicit def is phased out. They really don't play well with the implicit search optimizations, so risk to slow down compilation a lot.

@smarter
Copy link
Member

smarter commented Mar 12, 2020

We can't say B = Any since then the conversion would not fit anything. Instead we say it's B = <?>, where <?> is the wildcard type.

Could you point me to where in the code we do this?

@odersky
Copy link
Contributor

odersky commented Mar 13, 2020

@smarter It's in stripImplicit and normalize, where we call resultApprox. resultApprox actually creates fresh type variables for dependent parts fo the result type not wildcard types (I think it was wildcard types first, but that did not work out). That's more systematic, but still suffers the problem of vert large implicit searches.

@odersky
Copy link
Contributor

odersky commented Mar 24, 2020

In any case it's good to have this as a test, to document a possible scheme.

@odersky odersky merged commit 55e00c0 into scala:master Mar 24, 2020
@odersky odersky deleted the add-prototye-conversion-with-dependent-functions-suport branch March 24, 2020 09:42
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