TypeProviders: Add inner base-exception description

[Thorium]

As @smoothdeveloper PR #4978 from 2018 is still not not accepted, this tries to be the simplest possible fix for the existing issue #17249

[github-actions[bot]]

:heavy_exclamation_mark: Release notes required

---

:white_check_mark: Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

[Thorium]

Before this PR, from the current error message, I had no idea which file was missing. Tested with this PR's F# compiler and this actually helps telling the error (duckdb.dll missing in this case).

[abelbraaksma]

@Thorium About this comment: https://github.com/dotnet/fsharp/pull/17251#discussion_r1614970164

I just realize that GetBaseException() is virtual. Which means it could be overridden and, even though for built-in .NET exception classes, this does not misbehave anymore and has been fixed since, there's still the (slight) possibility that any derived class returns null, or something other than the deepest inner exception.

Which means, in turn, that your code is not safe currently for the same reasons you stated. I propose to get around this as follows, just add a function like the following, and use that instead:

[<TailCall>]
let rec getMostInnerException (ex: exn) =
    if isNull ex then 
        // should never happen, we are in a try-with (we may consider throwing instead of returning?)
        ArgumentNullException("Unreachable code. Caught exception is null", nameof ex) :> exn
    else
        if isNull ex.InnerException then ex
        else getMostInnerException ex.InnerException

---

Oh, and @smoothdeveloper had a https://github.com/dotnet/fsharp/pull/17251#discussion_r1614814873 that got buried a bit. to his point, I'd argue that the current change is already an improvement, but showing the entire stacktrace in the tooltip blob can easily take the whole screen and be disruptive. While I agree that it is useful, we may want to think about that a little first.

[Thorium]

No the ex it's not null itself, but the inner one is.

[abelbraaksma]

No the ex it's not null itself, but the inner one is.

@Thorium Yes, but that won't change how GetBaseException works (i.e., try (exn "foo").GetBaseException(), it doesn't have an inner exception, but works as expected and returns the current ex). It'll never return null even if InnerException is null.

My point was more about a user-overridden GetBaseException can only ever return null if it breaks the contract, but unfortunately, we don't know that, so we should not use it. Hence my suggestion above.

[edgarfgp]

Anything blocking this PR ? Looks a good improvement when working with TP cc @psfinaki

[psfinaki]

Well thanks @edgarfgp for reminding rather :)

Thanks @Thorium, merging!

[psfinaki]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).