Improve error message if dotnet is not found

[MangelMaxime]

Hello,

this PR fix #1709.

⚠️ Important: I had to disable Fantomas formatting because it generates invalid F# code. I opened a bug about it fsprojects/fantomas#2366

Changes:

• Rename LanguageService.dotnet to LanguageService.tryFindDotnet
• Improve LanguageService.tryFindDotnet to gives better errors message if dotnet is not found
• Remove Environement.dotnet and use LanguageService.tryFindDotnet instead
• Use a modal instead of the notification to display the error, this is because we cannot use multine in the notification making the error message not really readable
• New dotnet detection logic
• Return error message which differ depending on the context of the error. Did it occur because Fsharp.dotnetRoot is set or not.

Important

Please test on Windows, if the detection works when FSharp.dotnetRoot is set to a valid location. In order, to detect if the file exist at that location I am calling Environment.tryGetTool with the full computed path. On Linux, it executes which /home/mmangel/.dotnet/dotnet which return OK if found, but I don't know if the Windows equivalent was with full path too.

Test no needed anymore, I am now using the standard Note API to check if the file exist.

Questions

Generalise calling windows.showErrorMessage ?

Should we call windows.showErrorMessage everywhere where we handle an error result from calling LanguageService.tryFindDotnet? If yes, should we just call it directly from LanguageService.tryFindDotnet and make it return the dotnet path if found and failwith ... if not?

What do to with LanguageService.compilerLocation?

What does LanguageService.compilerLocation do?

In the old code if dotnet is not found, then Ionide try to use the compilerLocation which comes from the (LSP?) client. But how can the LSP client be running if dotnet is not found ?

If my understanding is correct, shouldn't we just remove it? --> Need to remove the commented code

If not, we need to re-add that portion of the logic. Waiting for some explanation to understand how to wire it.

Error preview

[Booksbaum]

doesn't work:

---

where doesn't work with a full path:

> where.exe "C:\Program Files\dotnet\dotnet.exe"
ERROR: Invalid pattern is specified in "path:pattern".

Requires instead:

> where.exe "C:\Program Files\dotnet:dotnet.exe"
C:\Program Files\dotnet\dotnet.exe

`where.exe /?`

WHERE [/R dir] [/Q] [/F] [/T] pattern...

Description:
    Displays the location of files that match the search pattern.
    By default, the search is done along the current directory and
    in the paths specified by the PATH environment variable.

Parameter List:
    /R       Recursively searches and displays the files that match the
             given pattern starting from the specified directory.

    /Q       Returns only the exit code, without displaying the list
             of matched files. (Quiet mode)

    /F       Displays the matched filename in double quotes.

    /T       Displays the file size, last modified date and time for all
             matched files.

    pattern  Specifies the search pattern for the files to match.
             Wildcards * and ? can be used in the pattern. The
             "$env:pattern" and "path:pattern" formats can also be
             specified, where "env" is an environment variable and
             the search is done in the specified paths of the "env"
             environment variable. These formats should not be used
             with /R. The search is also done by appending the
             extensions of the PATHEXT variable to the pattern.

     /?      Displays this help message.

  NOTE: The tool returns an error level of 0 if the search is
        successful, of 1 if the search is unsuccessful and
        of 2 for failures or errors.

Examples:
    WHERE /?
    WHERE myfilename1 myfile????.*
    WHERE $windir:*.*
    WHERE /R c:\windows *.exe *.dll *.bat
    WHERE /Q ??.???
    WHERE "c:\windows;c:\windows\system32:*.dll"
    WHERE /F /T *.dll

(Note: I'm using where.exe instead of where because I'm using powershell. And where is an alias for Where-Object)

So I suggest to keep directory separate:

let tryGetTool dir toolName =
    if isWin then
        let pattern =
            match dir with
            | Some dir ->
                $"{dir}:{toolName}"
            | None -> toolName
        spawnAndGetTrimmedOutput "where.exe" (ResizeArray [ pattern ])
        |> Promise.map (fun (err, path, errs) -> if path <> "" then Some path else None)
        |> Promise.map (
            Option.bind (fun paths ->
                paths.Split('\n')
                |> Array.map (String.trim)
                |> Array.tryHead)
        )
    else
        let tool =
            match dir with
            | Some dir -> path.join (dir, toolName)
            | None -> toolName
        spawnAndGetTrimmedOutput "which" (ResizeArray [ tool ])
        |> Promise.map (fun (err, path, errs) -> if path <> "" then Some path else None)

(also: no need to use cmd: where is an executable itself -> can be started directly)

[MangelMaxime]

@Booksbaum Thank you for checking.

I think I will go with the less fancy way and just use file.exist from Node API as it should be really cross platform. And it also convey the meaning of the check we are doing at this point in the code which is checking for the file existence.

This was actually my original idea.

[MangelMaxime]

I made the changes to use standard Node API to check for the file existence.

[MangelMaxime]

@baronfel This PR is ready to merge depending on the decision about:

Generalise calling windows.showErrorMessage ?

[baronfel]

I worry about the user experience of having tons of error modals fired. Is there a way we could detect not finding dotnet (until the configuration changes) and then try again? Ideally notifying the user via status bar or something that we're in a degraded state?

[MangelMaxime]

From what I understand you should only see the modal error once. At least, I had the modal only once when starting VSCode with an invalid path.

Because, if dotnet is not found nothing is executed.

Perhaps, if the user close the modal, and execute one of the command which invoke dotnet directly then he can see the model several times.

Workflow:

1. VSCode start
2. Error modal about dotnet not found
3. User close the modal
4. User use send FSI
5. Error modal about dotnet not found
6. User close the modal
7. etc.

[MangelMaxime]

Another ideas, I had this morning is:

Display a minimal notifications just saying "Could not find 'dotnet'" and had an action to the notification saying "More details".

When the user click on it, we open a preview panel with the detailed error message in it.

dotnet_not_found_md_demo.mp4

If we go this way, I think it makes sense to "generalise calling windows.showErrorMessage" in the tryFindDotnet function as it involve a bit of logic.

let dotnetNotFound (msg: string) =
    promise {
        let! selection = window.showErrorMessage ("Could not find 'dotnet'", "More details")

        match selection with
        | Some "More details" ->
            // Prepare a temporary file to store the error message
            let fileName = "ionide_dotnet_not_found_details_" + node.crypto.randomBytes(4).readUInt32LE(0).ToString() + ".md"
            let tempPath = node.path.join (node.os.tmpdir(), fileName)
            // Write the error message to the file, this will allow us to close the text editor 
            // without a confirmation dialog as the file content will not be modified.
            node.fs.writeFileSync(tempPath, msg)
            // Open the file in VSCode
            let newFile = vscode.Uri.parse (tempPath)
            let! document = workspace.openTextDocument newFile
            let! _ =  window.showTextDocument (document, ?options = None)
            // Show the error message in the markdown preview (better display)
            do! commands.executeCommand("markdown.showPreview", Some (box document))
            // Close the text editor (this does not close the markdown preview)
            do! commands.executeCommand("workbench.action.closeActiveEditor", Some (box document))
            // Delete the temporary file as it is not needed anymore.
            node.fs.unlinkSync(U2.Case1 tempPath)
        | Some _ 
        | None ->
            ()
        return failwith "no `dotnet` binary found"
    }

Edit: If you know a better way to display the detailed error message I am open to suggestion. I think there the possibility to register a custom TextDocumentContentProvider via workspace.registerTextDocumentContentProvider. But it seems a bit overkill/complicated for what we want.

Example: https://github.com/microsoft/vscode-extension-samples/tree/main/virtual-document-sample

[MangelMaxime]

Hello @baronfel @Krzysztof-Cieslak,

I rebased this PR to the latest version of main + improved the error display as show in my last message.

For me this PR is ready for review now.

[Krzysztof-Cieslak]

Thanks a lot! ❤️