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

Cleanup and improve realizability #5558

Closed
wants to merge 4 commits into from

Conversation

Blaisorblade
Copy link
Contributor

@Blaisorblade Blaisorblade commented Dec 2, 2018

The first part contains small changes and cleanups (mostly from #4036), except for 58aabb4 which is a more significant change.

The last commit refactors realizability significantly; it adds some complexity to avoid calling realizability(tp.info) twice, but otherwise clarifies and documents the code quite a bit, based on @abeln's patch.

Now a type is realizable if it selects a stable member on a realizable prefix — no exceptions. Which I did not know (and wasn't clear before #5521).

It might be the case that checking the prefix was in fact unnecessary in some cases, but we have neither documentation why that would be, testcases documenting the behavior, nor performance issues.

And in fact, even selecting a member with a singleton type from a path with mutable fields gives an unstable result, which I believe should hence be unrealizable.

@Blaisorblade Blaisorblade self-assigned this Dec 2, 2018
@Blaisorblade Blaisorblade force-pushed the fix-realizability branch 2 times, most recently from 9afd90b to 267031c Compare December 2, 2018 16:40
@Blaisorblade Blaisorblade changed the title WIP Fix realizability Cleanup realizability Dec 2, 2018
@Blaisorblade Blaisorblade assigned odersky and unassigned Blaisorblade Dec 2, 2018
@Blaisorblade Blaisorblade changed the title Cleanup realizability Cleanup and improve realizability Dec 2, 2018
@Blaisorblade Blaisorblade requested review from odersky and abeln December 2, 2018 18:15
/** A member is always stable if tp.info is a realizable singleton type. We check this last
for performance, in all cases where some unrelated check might have failed. */
def patchRealizability(r: Realizability) =
r.mapError(if (tp.info.isStableRealizable) Realizable else _)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@milessabin You asked whether def foo[L <: Lambda with Singleton]: L#Arg = ??? could work. That warrants an issue but here's a pointer, while I have this code fresh in memory. In this method we have code that considers realizable many things that are singletons of stable paths. The SingletonType branch below has similar logic.

The implementation would be different tho, since we'd have to handle type variables.

And as mentioned, this is blocked on Singleton not doing its job because of union types.

@milessabin
Copy link
Contributor

And as mentioned, this is blocked on Singleton not doing its job because of union types.

Linking to #4944 for future reference.

@odersky
Copy link
Contributor

odersky commented Dec 7, 2018

Needs a rebase

@odersky odersky assigned Blaisorblade and unassigned odersky Dec 9, 2018
* members from realizable types, that is, types having non-null values and
* not depending on mutable state.
* Beware that subtypes of realizable types might not be realizable; hence
* realizability checking restricts overriding. */
def realizability(tp: Type): Realizability = tp.dealias match {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blaisorblade what is the difference/relation between a type being stable vs being realizable? Consider adding that information to the method documentation.

I wrote this because I feared (incorrectly) exponential slowdowns in
realizability checking for this code. But debugging suggests that the
complexity of realizability checking is constant in the size of these
expressions (even after I disable caching of `Stable`).

Beware 1: these expressions almost smash the stack by sheer size.

Beware 2: this fails with `-Yno-deep-subtypes`, but simply because the checking
heuristics assumes people don't try to do this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants