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]

❗ Release notes required

---

✅ 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]

Great stuff! A few nits, nothing major. It would be nice if there were test cases for this... (but imo no need to add them now).

[abelbraaksma]

@Thorium About this comment: #17251 (comment)

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 #17251 (comment) 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.

[abelbraaksma]

My previous trains of thought may have been confusing, in part because I wasn't entirely correct. It is subtle. Please check my new comments, I think only the changes in line 108 are actually needed.

[edgarfgp]

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

[abelbraaksma]

@Thorium, thanks for taking the time to address my suggestions and patiently explaining your approach and choices.

I think one day we have to revisit e.GetBaseException(), as it skips over any and all exceptions between top and bottom, and I originally thought your PR was about getting the first inner exception of the tree. But that doesn't have to be addressed now.

Be as it may, I never meant to block this, but real life caught up and I wasn't notified anymore, until @psfinaki reminded me.

Let's get this in. I think it is a great improvement over what we had originally.

[psfinaki]

Well thanks @edgarfgp for reminding rather :)

Thanks @Thorium, merging!

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).