Fix #5521: prefix not checked in realizability check

[abeln]

We were forgetting to check the prefix in the uncached case.

[abeln]

@Blaisorblade do you have time to take a look? Guillaume mentioned on gitter that you were looking at CheckRealizable.

[Blaisorblade]

test performance please

[dottybot]

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

[Blaisorblade]

Kudos for the find! See comment; a further (oneliner) change is probably needed.

[Blaisorblade]

I looked at this time ago in #4036, I should look more closely, and I'm in a rush, but here are some initial comments.

I agree this is a bug and this change should give the correct behavior; but this change risks being pretty computationally expensive (possible fix below). This can duplicate recursive calls, and that risks causing exponential slowdowns. Triggering it might require a chain of final lazy vals or be impossible, but it's more robust to not duplicate calls.

If isLateInitialized(sym), this calls realizability(tp.prefix) again, and that can be too expensive. Caching only affects symbols, but we don't cache realizability on whole types; tp could not even be a TermRef and fall through boundsRealizability(tp).andAlso(memberRealizability(tp)).

I asked dottybot for a performance test. But maybe one could just reorganize the logic.
Looking at #4036 (which is where I "studied" this), I propose to add to your change the following:

-else if (!isLateInitialized(sym)) realizability(tp.prefix)
+else if (!isLateInitialized(sym)) Realizable

because in this branch we can certainly mark the symbol as stable (that PR also achieves this goal, though in a hackier way). If that works, this PR can go in with that fix.

Optional requests

Here's a couple of bonus requests one could fix here as well — if they are non-trivial, feel free to defer them. I should probably do these changes myself and PR them, I'm in a rush right now.

On performance: https://github.com/lampepfl/dotty/pull/4036/files#diff-0535d82f8f57a2204d6452bae575efc9 might be a useful test here (if it doesn't cause a stack overflow).

On error reporting, I think one could cherry-pick from #4036 the following:

-realizability(tp.prefix)
+realizability(tp.prefix).mapError(r => new ProblemInUnderlying(tp.prefix, r))

[dottybot]

Performance test finished successfully:

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

Benchmarks is based on merging with master (f66225c)

Addressed myself

[Blaisorblade]

@abeln To move things along I did the necessary changes myself — basic tests pass locally, let's see what the CI says. I've stuck to changes that really belong here to avoid scope creep.
EDIT: let me know what you think and if you want to do another pass, but otherwise we should be able to merge this.

[Blaisorblade]

test performance please

[dottybot]

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

[Blaisorblade]

Martin wrote:

If you are comfortable with #5522, yes please go ahead and merge it. Maybe run a performance test again, to be sure.

so doing that.

[dottybot]

Performance test finished successfully:

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

Benchmarks is based on merging with master (5d1efe3)

[abeln]

@Blaisorblade thanks for doing this, and sorry I didn't get to it myself! I got busy trying to get the flow-sensitive type inference to work for the null project (still working on it).