Skip to content

Fix handling of special implicit searches for ClassTag, quoted.Type, Eq #4436

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 5 commits into from
May 14, 2018

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 2, 2018

They did not work if the searched-for type was an uninstantiated TypeVar that
had the class as an upper bound. Getting them to work is surprisingly subtle.

@odersky
Copy link
Contributor Author

odersky commented May 2, 2018

Based on #4435

They did not work if the searched-for type was an uninstantiated TypeVar that
had the class as an upper bound. Getting them to work is surprisingly subtle.
@odersky odersky force-pushed the fix-specialized-implicits branch from ddbd04e to 4d95963 Compare May 2, 2018 09:34
Roll the functionality of contextualBaseType into baseType itself.
@odersky
Copy link
Contributor Author

odersky commented May 2, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 2, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

dottybot commented May 2, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4436/ to see the changes.

Benchmarks is based on merging with master (dfab5e5)

@odersky
Copy link
Contributor Author

odersky commented May 2, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 2, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

The previous logic tended to test things several times, which made
it harder to understand and possibly less efficient. We now combine
the cachability decisions in the main `recur` method of `baseTypeOf`.
@odersky
Copy link
Contributor Author

odersky commented May 2, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 2, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

odersky added 2 commits May 2, 2018 18:45
When testing on typer/*.scala, the differences are as follows:

                                    Cache all     Only cache successful
    Total basetypes                 386K          386K
    Full computations in recur      85K           397K
    Cache entries created           72K           46K
@dottybot
Copy link
Member

dottybot commented May 2, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4436/ to see the changes.

Benchmarks is based on merging with master (d0f7846)

@odersky
Copy link
Contributor Author

odersky commented May 2, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 2, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

dottybot commented May 2, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4436/ to see the changes.

Benchmarks is based on merging with master (d0f7846)

@odersky
Copy link
Contributor Author

odersky commented May 3, 2018

So this ended up rewriting baseTypeOf to be context dependent, i.e. baseTypeOf now takes constraint bounds for TypeParams into account. The first attempt was a bit messy so I ended up with a rewrite of baseTypeOf that does not caching and normal logic. The result is easier to understand and slightly more efficient than before.

@odersky odersky requested a review from smarter May 3, 2018 09:38
@odersky
Copy link
Contributor Author

odersky commented May 9, 2018

@smarter, can we get this in? It would make the Tasty extractors somewhat nicer to write.

@smarter
Copy link
Member

smarter commented May 9, 2018

Sure, I'll review this today.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM

val base = formalValue.baseType(cls)
if (base <:< formalValue) {
// With the subtype test we enforce that the searched type `formalValue` is of the right form
handler(base).orElse(ifNot)
Copy link
Member

Choose a reason for hiding this comment

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

In synthesizedClassTag there's a call to fullyDefinedType, I wonder if we should move this around here to do this consistently for all handlers.

@Blaisorblade Blaisorblade merged commit 9f876b8 into scala:master May 14, 2018
@Blaisorblade Blaisorblade deleted the fix-specialized-implicits branch May 14, 2018 18:51
@Blaisorblade
Copy link
Contributor

@nicolasstucki Enjoy ClassTags!

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