Confusion with paths detected by elm-review (windows OS)

[chemacortes]

With windows OS, messages from elm-review are not displayed into the editor, only messages from the elm LSP. With linux, all works fine.

Expected Behavior

With windows, elm-review seems not working with Visual Code and Elm extension.

Messages from LSP and elm-review are listed in the problems pane, but only messages from LSP are displayed into the code editor. Messages from elm-review are missing.

Current Behavior

I put in src/Main.elm an unused import:

import Html exposing (Html)

In the problems pane, I saw messages from LSP and elm-review:

LSP:

[{
	"resource": "/C:/Users/jmcortesa/temp/test-elmreview/src/Main.elm",
	"owner": "Elm",
	"severity": 4,
	"message": "Unused imported type `Html`",
	"source": "ElmLS",
	"startLineNumber": 4,
	"startColumn": 23,
	"endLineNumber": 4,
	"endColumn": 27,
	"tags": [
		1
	]
}]

elm-review:

[{
	"resource": "\\Users\\jmcortesa\\temp\\test-elmreview/src\\Main.elm",
	"owner": "Elm",
	"severity": 4,
	"message": "Imported type `Html` is not used",
	"source": "elm-review",
	"startLineNumber": 4,
	"startColumn": 23,
	"endLineNumber": 4,
	"endColumn": 27,
	"tags": [
		1
	]
}]

If in the problems pane, I click in the message from elm-review, I got an error message:

Unable to open 'Main.elm': Unable to resolve resource c:%5CUsers%5Cjmcortesa%5Ctemp%5Ctest-elmreview/src%5CMain.elm.

Something wrong is happening with the path.

Possible Solution

LSP and elm-review are using distint schemas for the path. Perhaps, vscode has facilities for homogenizing the use of routes.

Your Environment

• Visual Code: 1.66.2 (user setup)
• Elm: 0.19.1
• Elm-review: 2.7.2
• Node: 14.19.1
• Operating System and version: Windows 10 Pro

[razzeee]

Can you run elm-review --debug manually?

[chemacortes]

I use a minimal source file:

module Main exposing (..)

import Browser
import Html exposing (Html)


init =
    ""


view model =
    Html.text "Hello, World!"


update msg model =
    model


main =
    Browser.sandbox
        { init = init
        , view = view
        , update = update
        }

The output of elm-review --debug:

Microsoft Windows [Versión 10.0.19043.1645]

jmcortesa C:\Users\jmcortesa\temp\test-elmreview
$ npx elm-review --debug
Starting review application build
Compiling review application
Finished review application build
Building parser app for elm-syntax v7.2.9
Parsing using stil4m/elm-syntax v7.2.9
Reviewing the following files:
 - C:\Users\jmcortesa\temp\test-elmreview\src\Main.elm
-- ELM-REVIEW ERROR ------------------------------------------ src\Main.elm:1:22

(fix) NoExposingEverything: Module exposes everything implicitly "(..)"

1| module Main exposing (..)
                        ^^^^

Modules should have hidden implementation details with an explicit API so that
the module is used in a proper and controlled way. The users of this module
should not have to know about what is inside a module it is using, and they
shouldn't need to access its internal details. Therefore, the API should be
explicitly defined and ideally as small as possible.

────────────────────────────────────────────────────────────── src\Main.elm:4:23

(fix) NoUnused.Variables: Imported type `Html` is not used

3| import Browser
4| import Html exposing (Html)
                         ^^^^

You should either use this value somewhere, or remove it at the location I
pointed at.

─────────────────────────────────────────────────────────────── src\Main.elm:7:1

NoMissingTypeAnnotation: Missing type annotation for `init`

7| init =
   ^^^^
8|     ""

Type annotations help you understand what happens in the code, and it will help
the compiler give better error messages.

────────────────────────────────────────────────────────────── src\Main.elm:11:1

NoMissingTypeAnnotation: Missing type annotation for `view`

11| view model =
    ^^^^
12|     Html.text "Hello, World!"

Type annotations help you understand what happens in the code, and it will help
the compiler give better error messages.

────────────────────────────────────────────────────────────── src\Main.elm:11:6

NoUnused.Parameters: Parameter `model` is not used

11| view model =
         ^^^^^
12|     Html.text "Hello, World!"

You should either use this parameter somewhere, or remove it at the location I
pointed at.

────────────────────────────────────────────────────────────── src\Main.elm:15:1

NoMissingTypeAnnotation: Missing type annotation for `update`

15| update msg model =
    ^^^^^^
16|     model

Type annotations help you understand what happens in the code, and it will help
the compiler give better error messages.

────────────────────────────────────────────────────────────── src\Main.elm:15:8

NoUnused.Parameters: Parameter `msg` is not used

15| update msg model =
           ^^^
16|     model

You should either use this parameter somewhere, or remove it at the location I
pointed at.

────────────────────────────────────────────────────────────── src\Main.elm:19:1

NoMissingTypeAnnotation: Missing type annotation for `main`

19| main =
    ^^^^
20|     Browser.sandbox

Type annotations help you understand what happens in the code, and it will help
the compiler give better error messages.

Errors marked with (fix) can be fixed automatically using `elm-review --fix`.

I found 8 errors in 1 file.

[razzeee]

Would you be able to build #694 and test it?

Basically do https://github.com/elm-tooling/elm-language-client-vscode#contributing--debugging and switch the branch of the server repo

[chemacortes]

Sorry, but this is my first time debugging with vscode extensions.

I have not success with it. I paste the same message with elm-review as before:

[{
	"resource": "/jmcortesa/temp/test-elmreview/src/Main.elm",
	"owner": "Elm",
	"severity": 4,
	"message": "Imported type `Html` is not used",
	"source": "elm-review",
	"startLineNumber": 4,
	"startColumn": 23,
	"endLineNumber": 4,
	"endColumn": 27,
	"tags": [
		1
	]
}]

As you can see, the resource path is missing the prefix /C:/Users as do LSP:

[{
	"resource": "/c:/Users/jmcortesa/temp/test-elmreview/src/Main.elm",
	"owner": "Elm",
	"severity": 4,
	"message": "Unused imported type `Html`",
	"source": "ElmLS",
	"startLineNumber": 4,
	"startColumn": 23,
	"endLineNumber": 4,
	"endColumn": 27,
	"tags": [
		1
	]
}]

[razzeee]

Can you check if elm-review --debug --report=json has the same problem?

[chemacortes]

The output json of elm-review --debug --report=json has only one relative path: src\\Main.elm

@ C:\Users\jmcortesa\temp\test-elmreview
$ elm-review --debug --report=json
Starting review application build
Compiling review application
Finished review application build
Building parser app for elm-syntax v7.2.9
Parsing using stil4m/elm-syntax v7.2.9
Reviewing the following files:
 - C:\Users\jmcortesa\temp\test-elmreview\src\Main.elm
{
  "type": "review-errors",
  "errors": [
    {
      "path": "src\\Main.elm",
      "errors": [
        {
          "rule": "NoUnused.Variables",
          "message": "Imported type `Html` is not used",
          "ruleLink": "https://package.elm-lang.org/packages/jfmengels/elm-review-unused/1.1.22/NoUnused-Variables",
          "details": [
            "You should either use this value somewhere, or remove it at the location I pointed at."
          ],
          "region": {
            "start": {
              "line": 4,
              "column": 23
            },
            "end": {
              "line": 4,
              "column": 27
            }
          },
          "fix": [
            {
              "range": {
                "start": {
                  "line": 4,
                  "column": 13
                },
                "end": {
                  "line": 4,
                  "column": 28
                }
              },
              "string": ""
            }
          ],
          "formatted": [
            {
              "string": "(fix) ",
              "color": "#33BBC8"
            },
            {
              "string": "NoUnused.Variables",
              "color": "#FF0000",
              "href": "https://package.elm-lang.org/packages/jfmengels/elm-review-unused/1.1.22/NoUnused-Variables"
            },
            ": Imported type `Html` is not used\n\n3| import Browser\n4| import Html exposing (Html)\n                         ",
            {
              "string": "^^^^",
              "color": "#FF0000"
            },
            "\n\nYou should either use this value somewhere, or remove it at the location 
I pointed at."
          ],
          "suppressed": false,
          "originallySuppressed": false
        },

       ...
      ]
    }
  ]
}

[razzeee]

Well, we seem to be combining it with the workspace root path https://github.com/elm-tooling/elm-language-server/blob/main/src/providers/diagnostics/elmReviewDiagnostics.ts#L170

So I guess that's for some reason not correct? Still it uses that path before to check if the ReviewConfig.elm even exists :/

[chemacortes]

I understand the problem. workspaceRootPath is a URI like file:///c%3A/Users/jmcortesa/temp/test-elmreview. When it is converted into a string with the property .fsPath you get c:/Users/jmcortesa/temp/test-elmreview. That's right, but this string cannot be parse again into a uri because it take 'c:' as part of a protocol. Only .toString() method guarantees that the string can be parseable again into an uri as the original.

The fix is to keep workspaceRootPath as URI, use Utils.joinPath instead of path.join, and convert into strings with .fsPath or .toString() on demand:

--- a/src/providers/diagnostics/elmReviewDiagnostics.ts
+++ b/src/providers/diagnostics/elmReviewDiagnostics.ts
@@ -100,7 +99,7 @@ export class ElmReviewDiagnostics {
     const workspaceRootPath = this.elmWorkspaceMatcher
       .getProgramFor(filePath)
       .getRootPath();
-    return await this.checkForErrors(workspaceRootPath.fsPath);
+    return await this.checkForErrors(workspaceRootPath);
   };

   private hasType(error: any): error is IElmReviewError {
@@ -108,7 +107,7 @@ export class ElmReviewDiagnostics {
   }

   private async checkForErrors(
-    workspaceRootPath: string,
+    workspaceRootPath: URI,
   ): Promise<Map<string, IElmReviewDiagnostic[]>> {
     const settings = await this.settings.getClientSettings();
     const fileErrors = new Map<string, IElmReviewDiagnostic[]>();       
@@ -116,7 +115,8 @@ export class ElmReviewDiagnostics {
     if (
       settings.elmReviewDiagnostics === "off" ||
       !existsSync(
-        path.join(workspaceRootPath, "review", "src", "ReviewConfig.elm"),
+        Utils.joinPath(workspaceRootPath, "review", "src", "ReviewConfig.elm")
+          .fsPath,
       )
     ) {
       return fileErrors;
@@ -142,7 +142,7 @@ export class ElmReviewDiagnostics {
         elmReviewCommand,
         "elm-review",
         options,
-        workspaceRootPath,
+        workspaceRootPath.fsPath,
         this.connection,
       );
       return fileErrors;
@@ -166,10 +166,7 @@ export class ElmReviewDiagnostics {
           errorObject.type === "review-errors"
         ) {
           errorObject.errors.forEach(({ path, errors }: IFileError) => {
-            const uri = Utils.joinPath(
-              URI.parse(workspaceRootPath),
-              path,
-            ).fsPath;
+            const uri = Utils.joinPath(workspaceRootPath, path).toString();

             fileErrors.set(
               uri,

That works now with my windows installation.

[razzeee]

Thank you, seems to work fine like that on linux too.