emacs-lsp / lsp-treemacs

lsp-mode :heart: treemacs
GNU General Public License v3.0
396 stars 46 forks source link

lsp-treemacs-error not working properly on windows 10 #109

Open deb75 opened 3 years ago

deb75 commented 3 years ago

This issue is the following of #64

On windows 10, with latest lsp-treemacs, the command lsp-treemacs-error-list does return a tree, but unfolding items return nothing, whereas lsp-ui-flycheck-list does return correctly the errors.

All other lsp-treemacs commands appear to work well

deb75 commented 3 years ago

Hello,

No clue ?

I will test on linux, I wonder why it would work on linux and not on windows. How could it be platform dependent ?

A question of line ending (lf, crlf) ?

ulises commented 3 years ago

Same thing happens in mac os. Additionally, when trying to unfold an error tree I get:

let: Wrong type argument: char-or-string-p, nil
asmodeus812 commented 2 years ago

I am also facing the same issue, the nodes do not expand/unfold, @yyoncho any idea ?

yyoncho commented 2 years ago

@asmodeus812 open separate issue, we have to debug it. As a side note, I favour helm-lsp-diagnostics over lsp-treemacs tree view.

xuehy commented 2 years ago

Still not solved? I am facing the same issue.

cmburn commented 2 years ago

@yyoncho I think @deb75 and @asmodeus812 are referring to the same issue in that the nodes simply don't expand, which I've got as well. helm-lsp-diagnostics displays the errors correctly, so I'm guessing it's not an LSP issue

thomashilke commented 2 years ago

I am affected with the same issue under Windows 10, Emacs 29.0.50.

It turns out that the issue is in the function lsp-treemacs-errors--list-files at line 1189. The function lsp-f-ancestor-of? gets the folder of the workspace, and the path of a file that have diagnostics associated. On my machine, the case of the folder is different than the case of the file. For example, when debugging, I get in the *Message* buffer:

Result: "c:/[Redacted]/AcmeCorp.PlanarMachining"

Result: "c:/[redacted]/acmecorp.planarmachining/visualizers/curve2d/curve2dvisualizer.test/obj/debug/net48/curve2dvisualizer.test.assemblyinfo.cs"

So, even if the folder contains the file according to the NTFS/whatever filesystem, lsp-f-ancestor-of? returns nil if the casing doesn't match exactly.

If you patch the function lsp-f-canonical to downcase the filename like so:

(defun lsp-f-canonical (file-name)
  "Return the canonical FILE-NAME, without a trailing slash."
  (directory-file-name (expand-file-name (downcase file-name))))

Then the Error List buffer shows the files with the diagnostics correctly: the tree can be expanded, and diagnostics double-clicked to navigate to the faulty code.

The above fix is probably not the right thing to do, but I couldn't find the proper way to do it. Honestly, comparing paths should be something that is delegated to some OS api, and everything that is done in Elisp is an approximation of that, at least on Windows...

Anyway, I hope that my little analysis helps with fixing the issue for the Windows users!

MarcoHassan commented 2 years ago

Hi I am interested in the fix. Can you develop on it - in specific on the patch.

There is no function lsp-f-canonical in here.

Are you suggesting replacing lsp-f-ancestor.of? with it or what?

Thank you in advance.

xuehy commented 2 years ago

@MarcoHassan the function lsp-f-canonical is in lsp-mode.el

thomashilke commented 2 years ago

@xuehy yes @MarcoHassan Well, I did fix lsp-f-canonical naively inplace and reevaluate the defun. This modification actually breaks other important stuff.

I see 2 ways to approach the problem:

  1. Fix lsp-f-ancestor-of? since it is what fails here, or
  2. understand why the diagnostics hashtable returned by lsp-diagnostics hasn't the correct casing in the first place.

As long as you stay on Windows, a quick and dirty way to do it is to replace lsp-f-ancestor-of? with:

(defun lsp-f-ancestor-of? (path-a path-b)
  "Return t if PATH-A is an ancestor of PATH-B.
Symlinks are not followed."
  (unless (lsp-f-same? path-a path-b)
    (s-prefix? (concat (lsp-f-canonical (downcase path-a)) (f-path-separator))
               (lsp-f-canonical (downcase path-b)))))

Just edit-it in place in your .emacs.d/elpa folder and restart emacs. It will probably be overwritten a the next package update.

xuehy commented 2 years ago

@thomashilke yes we must make sure the code only runs on windows. Since I use emacs on both win and Linux, here's my quick dirty fix which can be used in both OS:

(defun lsp-f-ancestor-of? (path-a-raw path-b-raw)
  "Return t if PATH-A is an ancestor of PATH-B.
Symlinks are not followed."
  (let ((path-a (if (eq system-type 'windows-nt) (downcase path-a-raw) (path-a-raw)))
    (path-b (if (eq system-type 'windows-nt) (downcase path-b-raw) (path-b-raw))))

    (unless (lsp-f-same? path-a path-b)
      (s-prefix? (concat (lsp-f-canonical path-a) (f-path-separator))
             (lsp-f-canonical path-b)))))

It seems that only fixing lsp-f-ancestor-of? is still not enough with the error message lsp--on-idle wrong type argument stringp nil. Tracing the error, I found its root is function lsp-f-same?. Finally, my patch is:

(defun lsp-f-ancestor-of-patch (path-args)
  (mapcar (lambda (path) (downcase path)) path-args))

(when (eq system-type 'windows-nt) 
  (advice-add 'lsp-f-ancestor-of? :filter-args #'lsp-f-ancestor-of-patch)
  (advice-add 'lsp-f-same? :filter-args #'lsp-f-ancestor-of-patch))

which can be placed in your own init files and will not be overwritten when lsp packages get updated.

yyoncho commented 2 years ago

The above fix is probably not the right thing to do, but I couldn't find the proper way to do it. Honestly, comparing paths should be something that is delegated to some OS api,

There are OS calls(file-truename) but we avoid them on purpose because they are pretty much killing lsp-mode performance. Thus all these ugly hacks.

I will accept this patch as a PR.