fix 5580 and better encapsulate constraint solver #8294
Conversation
|
cc @gusty |
Just to note that this was first introduced as public in the VS 2017 GA release. |
|
Generally I think these tests would live better in the NUnit test suite we have that VS will load and run in the IDE, using the That said, since these aren't additions to fsharpqa (and thus failures can also be tracked in CI infrastructure) I won't exactly screech if they're checked in as-is. |
|
@cartermp if you are refering to the neg116 and others https://github.com/dotnet/fsharp/pull/8294/files#diff-e1f85d74b97d6a85e865cf2bc21a142d those are already runnable as normal nunit test with the IDE tooling you are refering to, and the tests are not part of fsharpqa tooling. It is not true that they are easier to maintain in literal strings and checking against FSharpError objects, what you say is relelvant for tests that check that code compiles, but not for tests verifying error messages / warnings. |
They are not available in Solution Explorer, which makes them more of a chore to maintain. So we're going to agree to disagree on this point.
See above comment where this was already acknowledged |
|
It would be interesting to note on the test (maybe as a comment) whether in a super-ideal situation the code should compile or not, regardless of compiler limitations (technicals, not specs) and history. This is useful when the code we're testing is a bit obscure, so if such test wasn't compiling and after a change it compiles, we will wonder whether is a good thing or bad, as looking at the testing code is not obvious. I do this in F#+ to track regressions that are good or bad and explaining why, for instance if a piece of code doesn't compile, why is it good that it doesn't. Although in my case the testing code is simpler but I rather do it because the error messages I get are not very helpful. |
The problem with this approach is that in some of the exceptionally complex inference cases, I wouldn't necessarily trust my judgement without a backing "inference calculator" to check my calculations, ie. an implementation of type checking. So what's needed to achieve that would be an ideal re-implementation of the type checker. And if such a thing diverged from current inference the result would not be particularly actionable. So for corner cases associated with SRTP the implementation effectively acts as the specification. The particular places where this doesn't apply are if inference is unsound. |
|
@cartermp @TIHan @KevinRansom I'd appreciate a review on this, as I'd like to get the fix finalized and in to master, to integrate into #6805 |
There was a problem hiding this comment.
Ok, I went through each line of change and it looks fine to me.
AFAIK, majority of the code change was encapsulating constraint solver by refactoring.
The fix for the issues seemed to be the introduction of TryD_IgnoreAbortForFailedOverloadResolution which takes into account AbortForFailedOverloadResolution - the use of it is at entrypoints that were using TryD before.
|
@brettfo, any idea why this is failing? |
That's correct :) |
|
More unrelated failures in the source build We've seen this before, suggestion is to update SourceLink |
* fix 5580 and better encapsulate constraint solver * fix 5580 and better encapsulate constraint solver * fix 5580 and better encapsulate constraint solver * fix 5580 and better encapsulate constraint solver * add new tests * nudge CI
As part of working on testing for #6805 I encountered bugs #5580 and #7217
This caused me to take a long look at the problem of
LocallyAbortOperationThatFailsToResolveOverloadThis local-abort is a thing (possibly a mistake) which has crept into F# type inference,
and I regard its status is currently under review, though it can't just be removed as it affects the
specification of constraint solving in type inference.
The two linked bugs are cases where this exception was escaping. This happened because only two out of bout 10 "entry points" into the constraint solver (i.e. calls that may affect constraint state) were protected for this exception being raised.
Here is the whole sordid history of this (I've left this note in the codebase too)
The local abort was added as part of an attempted performance optimization Speed up Constraint Solver's overload resolution (II) #1650. This change was released somewhere in VS2015.
However, the local abort inadvertently also impacted the logic of type inference, by skipping some checking (which allows more things to type check, possibly unsoundly, see below). Because of this an attempt was made to revert it in Revert #1650 (and #3366) due to regressions #4173.
Unfortunately, existing code had begun to depend on the new behaviours enabled by the change, and the revert was abandoned before release in Revert of #4173 in dev15.6 #4348
As an aside, I'll mention that I'm skeptical that the addition of the local abort is sound. It is ok in the vast majority of cases, because the method constraint will be subject to further processing at a later point. However in complex cases the local abort may surely result in the skipping
other processing associated with the assertion of some overall constraint (e.g. when processing a tuple constraint the processing later elements the tuple may be skipped)
However, we can't just remove this local abort because, as mentioned, it affects some cases of type inference.
In any case, this PR does these:
Resolves the two immediate bugs where where the local abort was escaping the constraint solver and a catastrophic failure ensued.
Tightens up the encapsulation for the constraint solver by making it impossible to create
ConstraintSolverEnvanywhere but inConstraintSolver.fs. This means that we can check that all the entry points are protected for the creation of this local abort condition.I've added the two test cases for the failing bugs.
This is a fix for the product and also unblocks work and testing for #6805