Add pattern type check recovery #7711

auduchinok · 2019-10-08T14:56:41Z

The problem this PR solves is almost every pattern type check error currently raises an exception making the rest of the match expression not type checked at all (so nothing of Highlighting, Find Usages, Rename, etc work). An example with named arguments for an active pattern (which is not allowed):

This PR adds recovery to the most of such errors making IDE features work as expected:

The difference is especially evident on match expressions with many clauses, due to all subsequent clauses were skipped on an error.

A naive approach to recover in most cases would be to produce typed Wild patterns (as it's already done in few places) but, due to how match expressions and bindings are compiled to decision trees, in many cases it'd make subsequent match clauses considered never matched, which doesn't look good:

This PR uses a different approach and introduces a new typed pattern for error and a fake Error decision tree test. These added parts of decision trees never match other parts of the tree so errors never make other parts considered never matched.

A tricky part here is extra patterns in the case of union case fields and literals. To prevent any effect on resulting decision tree, the arguments are type checked with fresh inference types and resulting names added to type check environment without creating new typed patterns. Due to variables for these patterns are not present in the tree, dummy unit expressions are created as initializers so Pattern Match Compilation doesn't fail to find them.

An example for unresolved function arguments:

Other cases producing errors, like patterns with attributes or optional values, are all supported too:

…cking args

Krzysztof-Cieslak · 2019-10-08T16:32:01Z

❤️

forki · 2019-10-08T16:33:47Z

Cool stuff

auduchinok · 2019-10-08T17:50:24Z

Needs some baseline updates.

dsyme

This looks really good, though I'm concerned about the use of catch-all error handlers, are they certainly necessary, or are they for recovery after reported errors?

src/fsharp/TypeChecker.fs

auduchinok · 2019-10-09T07:47:23Z

@dsyme These catch-all and forget handlers were added to places where it could fail and we couldn't recover afterwards. I could try listing particular exceptions but in some cases it'd probably be a long list, and since we don't need exception info at recovery point I just don't even look at what exception is.

dsyme · 2019-10-10T01:21:48Z

@dsyme These catch-all and forget handlers were added to places where it could fail and we couldn't recover afterwards. I could try listing particular exceptions but in some cases it'd probably be a long list, and since we don't need exception info at recovery point I just don't even look at what exception is.

I feel we do need to dig into this. IIRC the right pattern is this one:

    try something
    with e -> 
        errorRecovery e someRange
        error-result

For example, TcConst may raise error without recovery. Callers to TcConst add recovery via the above. Normally this will result in the error-result but in some cases like StopProcessingException in F# Interactive the error propagation will continue.

auduchinok · 2019-10-29T12:34:45Z

@dsyme I've added errorRecovery and tried to minimize the diff.

auduchinok · 2020-03-04T16:13:27Z

It's ready.
There's a failing SourceBuild_Linux build remaining but it doesn't seem to be related, does it?

cartermp · 2020-03-04T16:28:40Z

It's unrelated. The failures there are CI unreliability problems that we have on our .NET linux pools.

TIHan · 2020-03-06T01:55:38Z

tests/fsharp/typecheck/sigs/neg16.bsl

@@ -63,12 +63,16 @@ but here has type

 neg16.fs(85,8,85,18): typecheck error FS0039: The pattern discriminator 'FooA++' is not defined.

+neg16.fs(85,7,85,22): typecheck error FS0025: Incomplete pattern matches on this expression.


I know this is minor, but the code for this test:

let (|``FooA++``|) (x:int) = x let (``FooA++``(x)) = 10

Normally it would error saying that FooA++ is not defined because using "++" in the name is not allowed. But now it will include an additional error saying that the pattern matches are incomplete, but it isn't a valid match construct to begin with.

Again, this is minor and not a blocker.

TIHan · 2020-03-06T02:06:40Z

tests/fsharpqa/Source/Conformance/DeclarationElements/LetBindings/Basic/E_AttributesOnLet01.fs

@@ -4,7 +4,10 @@
 // Regression test for FSharp1.0:3744 - Unable to apply attributes on individual patterns in a tupled pattern match let binding - Implementation doesn't match spec
 // Test against error emitted when attributes applied within pattern

-//<Expects id="FS0683" span="(11,6-11,33)" status="error">Attributes are not allowed within patterns</Expects>
+//<Expects id="FS0683" span="(14,6-14,27)" status="error">Attributes are not allowed within patterns</Expects>
+//<Expects id="FS0842" span="(14,8-14,23)" status="error">This attribute is not valid for use on this language element</Expects>


This is better considering it is now showing multiple errors on both attributes along with better ranges.

The only minor knit I have is that it is also giving an error saying the "attribute is not valid for use on this language element" - but the real error is that attributes are just not allowed in patterns.

TIHan · 2020-03-06T02:10:42Z

tests/fsharpqa/Source/Conformance/PatternMatching/Union/E_UnionCapturesDiffType01.fs

@@ -1,6 +1,6 @@
 // #Regression #Conformance #PatternMatching #Unions
 // Verify error if  two union rules capture values with different types
-//<Expects id="FS0001" status="error" span="(8,7-9,11)">This expression was expected to have type.    'int'    .but here has type.    'float'</Expects>
+//<Expects id="FS0001" status="error" span="(9,10-9,11)">This expression was expected to have type.    'int'    .but here has type.    'float'</Expects>


This is a much more accurate range :)

TIHan · 2020-03-06T02:22:01Z

src/fsharp/TypeChecker.fs

+            | _ -> ())
+
+        let names = NameMap.layer names1 names2
+        let takenNames = Set.union takenNames1 takenNames2


What is the reason for doing a layer and union here? Could you put in a comment explaining why?

TIHan · 2020-03-06T02:27:23Z

src/fsharp/TypeChecker.fs

                    // so type T = Case of name: int * value: int
                    // | Case(value = v)
                    // will become
                    // | Case(_, v)
                    let result = Array.zeroCreate numArgTys
+                    let extraPatterns = List ()


What is extraPatterns used for? Could put in a comment explaining that?

TIHan

Looks good and a great improvement to error recovery! Only a few comments.

The only thing I would really like is adding more comments in the code as I noted.

KevinRansom · 2020-03-06T05:43:16Z

@auduchinok, if you can submit a quick PR with the additional clarifications @TIHan asked for please.

Thanks

KEvin

TIHan · 2020-03-06T05:43:19Z

@auduchinok I know the PR is merged, but I would really like the comments added to the code that I noted. Is it ok that you create a separate PR for that?

auduchinok · 2020-03-06T07:00:53Z

@TIHan sure, will do.

* TcPat recovery: simple pats * TcPat recovery: report union case and active pattern items before checking args * TcPat recovery: type check attributes * TcPat recovery: literal patterns, union case named args * TcPat recovery: literal patterns, union case args * TcPat recovery: active patterns named args * TcPat recovery: union case 2 * TcPat recovery: update tests * TcPat recovery: different names/types in Or patterns * Move tests file to common place * Fix tupled union case args * Lost change * Fix test project file include * Review fix: use errorRecovery when catching exceptions * Minimize diff * Minimize diff * Update FCS project analysis test baselines * Merge fixes * Update test * Fix fsharpqa baselines * Another fsharpqa baselines update * Update desktop suite baselines * Update vsbsl

dsyme · 2022-09-09T18:38:55Z

src/fsharp/TypeChecker.fs

+                    if numArgs < numArgTys then
+                        if numArgs <> 0 && numArgTys <> 0 then
+                            errorR (UnionCaseWrongArguments (env.DisplayEnv, numArgTys, numArgs, m))
+                        args @ (List.init (numArgTys - numArgs) (fun _ -> SynPat.Wild (m.MakeSynthetic()))), extraPatterns


This code is too complex - all the cases could do with some comments

auduchinok added 10 commits October 8, 2019 16:08

TcPat recovery: simple pats

0aa662e

TcPat recovery: report union case and active pattern items before che…

9d15228

…cking args

TcPat recovery: type check attributes

d6e3252

TcPat recovery: literal patterns, union case named args

0c68b3e

TcPat recovery: literal patterns, union case args

e059203

TcPat recovery: active patterns named args

d630e07

TcPat recovery: union case 2

df5fff1

TcPat recovery: update tests

bb108bb

TcPat recovery: different names/types in Or patterns

4624f58

Move tests file to common place

761c242

auduchinok changed the title ~~Add pattern type check recovery~~ [WIP] Add pattern type check recovery Oct 8, 2019

auduchinok added 2 commits October 8, 2019 19:02

Fix tupled union case args

40a3017

Lost change

69e8abb

cartermp requested review from TIHan and dsyme October 8, 2019 16:41

Fix test project file include

4747618

auduchinok changed the title ~~[WIP] Add pattern type check recovery~~ Add pattern type check recovery Oct 8, 2019

dsyme reviewed Oct 9, 2019

View reviewed changes

auduchinok added 3 commits October 29, 2019 15:29

Review fix: use errorRecovery when catching exceptions

3007d08

Minimize diff

dab1d30

Minimize diff

213de15

Update FCS project analysis test baselines

2debbc1

auduchinok closed this Feb 27, 2020

auduchinok reopened this Feb 27, 2020

auduchinok added 7 commits March 4, 2020 01:05

Merge remote-tracking branch 'origin/master' into tcPat-recovery

04e0378

Merge fixes

87b70da

Update test

6de27d8

Fix fsharpqa baselines

49c6774

Another fsharpqa baselines update

ad6bf76

Update desktop suite baselines

d28b56e

Update vsbsl

c37f824

dsyme approved these changes Mar 4, 2020

View reviewed changes

KevinRansom approved these changes Mar 5, 2020

View reviewed changes

TIHan reviewed Mar 6, 2020

View reviewed changes

TIHan approved these changes Mar 6, 2020

View reviewed changes

KevinRansom merged commit d1a3d07 into dotnet:master Mar 6, 2020

auduchinok mentioned this pull request May 28, 2020

FS0192 Internal Error with trivial code: DecisionTreeTest.Error should only be used during pattern match compilation #9296

Closed

KevinRansom mentioned this pull request Jun 11, 2020

Fix recovery #9420 #9426

Merged

auduchinok mentioned this pull request Jun 22, 2022

Wrong type is reported in type mismatch error #13347

Merged

auduchinok deleted the tcPat-recovery branch September 9, 2022 17:30

dsyme reviewed Sep 9, 2022

View reviewed changes

auduchinok mentioned this pull request Dec 6, 2023

Checker/patterns: recover on unifying union types #16393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pattern type check recovery #7711

Add pattern type check recovery #7711

auduchinok commented Oct 8, 2019 •

edited

Loading

Krzysztof-Cieslak commented Oct 8, 2019

forki commented Oct 8, 2019

auduchinok commented Oct 8, 2019

dsyme left a comment

auduchinok commented Oct 9, 2019 •

edited

Loading

dsyme commented Oct 10, 2019 •

edited

Loading

auduchinok commented Oct 29, 2019

auduchinok commented Mar 4, 2020

cartermp commented Mar 4, 2020

TIHan Mar 6, 2020

TIHan Mar 6, 2020

TIHan Mar 6, 2020

TIHan Mar 6, 2020

TIHan Mar 6, 2020

TIHan left a comment

KevinRansom commented Mar 6, 2020

TIHan commented Mar 6, 2020

auduchinok commented Mar 6, 2020

dsyme Sep 9, 2022

		@@ -63,12 +63,16 @@ but here has type

		neg16.fs(85,8,85,18): typecheck error FS0039: The pattern discriminator 'FooA++' is not defined.

		neg16.fs(85,7,85,22): typecheck error FS0025: Incomplete pattern matches on this expression.

Add pattern type check recovery #7711

Add pattern type check recovery #7711

Conversation

auduchinok commented Oct 8, 2019 • edited Loading

Krzysztof-Cieslak commented Oct 8, 2019

forki commented Oct 8, 2019

auduchinok commented Oct 8, 2019

dsyme left a comment

Choose a reason for hiding this comment

auduchinok commented Oct 9, 2019 • edited Loading

dsyme commented Oct 10, 2019 • edited Loading

auduchinok commented Oct 29, 2019

auduchinok commented Mar 4, 2020

cartermp commented Mar 4, 2020

TIHan Mar 6, 2020

Choose a reason for hiding this comment

TIHan Mar 6, 2020

Choose a reason for hiding this comment

TIHan Mar 6, 2020

Choose a reason for hiding this comment

TIHan Mar 6, 2020

Choose a reason for hiding this comment

TIHan Mar 6, 2020

Choose a reason for hiding this comment

TIHan left a comment

Choose a reason for hiding this comment

KevinRansom commented Mar 6, 2020

TIHan commented Mar 6, 2020

auduchinok commented Mar 6, 2020

dsyme Sep 9, 2022

Choose a reason for hiding this comment

auduchinok commented Oct 8, 2019 •

edited

Loading

auduchinok commented Oct 9, 2019 •

edited

Loading

dsyme commented Oct 10, 2019 •

edited

Loading