Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix tide failure in magit buffers. #380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix tide failure in magit buffers. #380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions tide.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2088,6 +2088,15 @@ current buffer."
tide--minimal-emacs emacs-version)
:error))

;; When a user uses magit to extract old commits of files, magit creates
;; buffers for those commits. These buffers are not backed by actual
;; files. However, magit "cheats" and sets `buffer-file-name' when it turns on
;; the modes on the file. So when this code runs in such a buffer,
;; `buffer-file-name' is set, even though the buffer is not backed by a file
;; on disk. So we need this check to handle such cases.
Comment on lines +2094 to +2096
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a description of a better strategy instead of hard-wiring it to just magin? That is, test whether the file exists on disk, and abort if it isn't?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do support org-mode which uses indirect buffer for chunks of code and might not be linked to a file. But this is very limited and works only for code without any dependencies (aka single file). In the case of magit following the same approach won't work because magit might show old version of code which might not compile without other dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elibarzilay Besides what ananthakumaran said, there's the issue of how you determine that tide-setup must give up.

So far the discussion in issues here and the comments in the code has distinguished between "buffers backed by files" and "buffers not backed by files", but that's not quite what the issue is. The real issue is whether a buffer has a file name associated with it or not. When we ask Emacs to open a file that does not exit yet, Emacs open a buffer for it, associates the buffer with the file name we asked for, but does not create the file on disk yet. (Note that this specifically precludes using "test whether the file exists on disk" as a test. Otherwise, tide would fail to start in cases where it should.) Though this buffer is not yet backed by a file, it is possible to edit it and get support from tsserver. Eventually, we'll save the file, but whether the buffer has a file that exists on disk or not is not the determining issue. The determining issue is whether or not the buffer has a file name associated with it. tsserver needs to know what the file name of the module is going to be, and so it does not work with buffers that have no file names associated with them.

AFAIK, the standard way to check whether a buffer has a file name associated with it is to check whether buffer-file-name is non-nil. But as I've noted in the comment, magit does set this variable. So by the time tide-setup runs, the buffer has a file name associated with it, and tide-setup does not detect the problem.

Magit sets this variable so that when an old commit is extracted, the editing modes that one would normally expect to see when editing the file are turned on. Since mode selection depends (among other things) on file names, I don't see magit has having any choice here: if it wants to turn on the modes and get sensible results, it has to set buffer-file-name to something sensible.

I generally want to avoid this kind of very narrow case-specific detection, but I'm not seeing a reliable solution here that does not require knowing we're dealing with magit specifically. The more general code path is the code that was already there prior to my changes ( (unless (stringp buffer-file-name) ...) but that code path is not adequate for dealing with magit.

Do you have another suggestion than checking whether the file exists on disk? (As explained above, it won't work.)

(when (bound-and-true-p magit-buffer-file-name)
(error "Cannot run tide in a buffer created by magit"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this throwing an error? Nothing really went wrong, it should just disable whatever it can't do, but not throw an error...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"whatever it can't do" It cannot do anything generally useful. Oh, it is always possible to come up with a small test case where things look fine, but as you start trying more and more complex scenario, tide cannot handle it, primarily because tsserver cannot handle it.

As mentioned in a previous PR comment, error' was expedient for a proof-of-concept. I suppose the mode *could* just silently abort, but that's not something I'm in favor of, as it would leave users puzzling over why typescript-modeis on but nottide`.

I'm edging now towards display-warning and returning.


;; Indirect buffers embedded in other major modes such as those in org-mode or
;; template languages have to be manually synchronized to tsserver. This might
;; cause problems in files with lots of small blocks of TypeScript. In that
Expand All @@ -2096,6 +2105,7 @@ current buffer."
(unless (stringp buffer-file-name)
(setq tide-require-manual-setup t))


(set (make-local-variable 'eldoc-documentation-function)
'tide-eldoc-function)
(set (make-local-variable 'imenu-auto-rescan) t)
Expand Down