Prune constraints that could turn into cycles

[odersky]

Fixes #864. Review by @smarter.

[odersky]

The issue uncovered a much deeper problem with constraint solving, which we solve by deliberatley widening narrowing the constraint.

[smarter]

Interesting approach, do you think that you could create a neg test that demonstrate valid code that does not compile with this patch?
I see that you added an optional cycle detection check in addOneBound, couldn't we leave addConstraint as it was before, and instead always check for cycles in addOneBound and heal them? Would that be too costly?

[odersky]

@smarter Logically, prune belongs where it is, because that's where we split a bound into a bound without type params and additional parameter/parameter relationships established by addLess. The cycle check in addConstraint can still fail, because TypeVars might be instantiated late.

[odersky]

@smarter Also note that the cycle check does not catch cycles of length > 1.

[smarter]

What about narrowing a bit less by using something like prune(constraint.nonParamBounds(bound).lo) instead of defn.NothingType?

[odersky]

That would not add anything. By recording the isLess we get the bounds anyway.

[smarter]

Should we add a debug log message when we narrow bounds? Otherwise it might be easy to overlook when trying to understand why some code does not compile.

[odersky]

I don't think we need a log message. The cases where this will fail will be really rare, and should be easily caught already by turning on the constr printer. Without constr, I think t will be impossible to diagnose.

[smarter]

You say "another constrained parameter" but the check does b eq param, so shouldn't you say something like "Make sure none of the bounds of a parameter in an OrderingConstraint contains this parameter at its toplevel" ?

[odersky]

yes, indeed. Can you make that change please? Thanks!

[smarter]

Done, also fixed the documentation of addConstraint.