[WIP] fix for #4956 (TP error reporting)

[smoothdeveloper]

issue #4956: Make Type Provider troubleshooting easier with better error reporting

Let the full exception details surface when a type provider fail at design / compile time.

Looking at the CI to get an idea of tests that need adjusting.

[smoothdeveloper]

I'm checking the test output, the issue is that now the output contains the file path where the code was compiled from in the stacktrace, it looks like the tests are comparing the output for exact match.

For example:

neg1.fsx(3,18,3,70): typecheck error FS0001: This expression was expected to have type
    'string'    
but here has type
    'int'    

neg1.fsx(9,38,9,64): typecheck error FS3021: Unexpected exception from provided type 'FSharp.EvilProvider.IsArrayTypeRaisesException' member 'IsArray': The type provider 'Provider.EvilProvider' reported an error: System.Exception: deliberate error for testing purposes

   at <StartupCode$provider>.$Provider$fsx.Microsoft-FSharp-Core-CompilerServices-IProvidedNamespace-ResolveTypeName@130.Invoke(Unit _arg1)

   at System.Type.get_IsArray()

   at Microsoft.FSharp.Compiler.Tainted`1.Protect[a](FSharpFunc`2 f, range range) in C:\dev\projects\github.com\microsoft\visualfsharp4\src\fsharp\tainted.fs:line 94

notice C:\dev\projects\github.com\microsoft\visualfsharp4\src\fsharp\tainted.fs is part of the output.

How should I go about adjusting the tests with baseline?

[dsyme]

@smoothdeveloper I suppose you'll need to modify the code that compares the output of the test andd the baseline

[dsyme]

Somewhere around here I suppose: https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/test-framework.fs#L448

So you should first update the baselines:

1. run the tests on your local machine
2. delete the *.bsl and *.vsbsl files for the failing tests and replace them by *.err and *.vserr)

There is already code these to diff out the current directory so perhaps you might not need to change anything

The actual call to the diffing routing is made here: https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/single-test.fs#L215

Note you can run the tests from F# interactive

1. build net40
2. send the top 28 lines from https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/tests.fs to F# Interactive
3. Run test fragments like ``singleNegTest (testConfig "typecheck/sigs") "neg1"`

[smoothdeveloper]

This is now passing, one drawback I see is for maintaining the tests in the future:

• they assume windows path, I've kept the existing substitution logic (.bslpp), if that becomes an issue, we will need to improve that part a bit
• if some of the fsharp code present in the stackframe changes, those tests will show the diff

I've gathered some information into a readme.md file that we should improve with more description and tips.

https://github.com/smoothdeveloper/visualfsharp/blob/a30a8da6a5c474f18f262f693f6726a979b464ab/tests/fsharp/readme.md

This comment gives an example of actual stacktrace: #4956 (comment) it will help type provider developers to set break points in the type provider SDK and figure out what is the problem they are facing.

[smoothdeveloper]

This shouldn't be merged for now: #4956 (comment)

[dsyme]

@KevinRansom @cartermp I'd like some advice on this one

In general we don't want to show stack traces in error messages. But we do need to improve the diagnostics form TP developers and TP early adopters so they can see stack traces if they want.

So we should really have some kind of mechanism to "turn on stack traces for TP diagnostics". But what should it be? A VS IDE switch and command line flag seems too heavy but maybe it's what we need.

thanks

[KevinRansom]

Can I get back to you on this ...

[dsyme]

Can I get back to you on this ...

Yes, absolutely no rush

[baronfel]

Something along these lines would be amazing. For TPs I work on I usually end up having a debug function that spams logs and catches/dumps stacktraces before rethrowing while I'm doing active development. Anything we can do to surface TP errors to TP developers would help demystify the whole process.

[smoothdeveloper]

this is impacting fsprojects/FSharp.Data.SqlClient#262

I tend to agree that surfacing the whole stack trace is too crude and will freak out end users while this is, in essence, a library bug.

Anybody has idea how we could still get the full trace in DEBUG builds of provider? maybe a specifc define symbol that the compiler uses for TP design time assemblies, and make fsc/fsi pick the verbose message (corresponding to my PR)?

This will definitely help TP authoring, but we want to avoid kilometer long error by any means in release builds for TP consumers.

[smoothdeveloper]

In general we don't want to show stack traces in error messages. But we do need to improve the diagnostics form TP developers and TP early adopters so they can see stack traces if they want.

Can we ease such decision by enabling the compiler to show stacktraces behind a flag like --surface-internal-errors?

this won't change anything for existing use of the compiler but provide easy escape hatch when this would really help diagnosis in various circumstances (mainly TP, compiler and tooling internal errors).

[KevinRansom]

@dsyme , @smoothdeveloper , hey guys, is this PR going anywhere, I am trying to reduce the orphaned PR's in our repo.

Kevin

[smoothdeveloper]

@KevinRansom I'm still in favor of having full stack trace to appear.

This is @dsyme comment:

exceptions are how TPs currently raise diagnostics, and we can't attach stack traces to those diagnostics.

[KevinRansom]

@dsyme , @smoothdeveloper , hey guys, is this PR going anywhere, I am trying to reduce the orphaned PR's in our repo.

Kevin

[KevinRansom]

I discussed this with @cartermp and we decided to close it, we should address stack merging of inner exceptions somehow, but probably not this way.