Sarcasm / irony-mode

A C/C++ minor mode for Emacs powered by libclang
GNU General Public License v3.0
901 stars 98 forks source link

Fix a bug when new files are created for the first time #479

Closed hotpxl closed 6 years ago

hotpxl commented 6 years ago

Bug Reproduction

  1. Have Irony and Flycheck enabled
  2. Create a new file using find-file
  3. Write some text and save the file

Environment

Emacs 26.1 RC1 (prebuilt by https://emacsformacosx.com/) macOS High Sierra Version 10.13.4

What Happens

Flycheck gives out the following error in *Messages* buffer:

Error from syntax checker irony: Bad I/O task data: #s(irony-iotask-packaged-task (:start (lambda (process buffer) (if (assq buffer (process-get process :unsaved-buffers)) (irony--server-send-command "reset-unsaved" (irony--get-buffer-path-for-server buffer)) (irony-iotask-set-result t))) :update irony--server-command-update :finish (lambda (process buffer) (process-put process :unsaved-buffers (assq-delete-all buffer (process-get process :unsaved-buffers))))) (# #) #s(irony-iotask-result error nil irony-iotask-bad-data (#0 "(error . (no-such-entry \"failed reset unsaved buffer\" \"/tmp/abcd.cpp\")

;;EOT ")) nil #s(irony-iotask-packaged-task (:start (lambda (file compile-options) (apply (function irony--server-send-command) "parse" file "--" compile-options)) :update irony--server-command-update) ("/tmp/abcd.cpp" ("-x" "c++")) #s(irony-iotask-result nil nil nil nil) nil #s(irony-iotask-packaged-task (:start (lambda nil (irony--server-send-command "diagnostics")) :update irony--server-query-update) nil #s(irony-iotask-result nil nil nil nil) nil nil))), "(error . (no-such-entry \"failed reset unsaved buffer\" \"/tmp/abcd.cpp\")

;;EOT "

And the Irony log file has the following content:

execute: Command{action=Command::Parse, file='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T//irony-server-0895e5', unsavedFile='', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=['-x', 'c++'], opt=off} execute: Command{action=Command::Diagnostics, file='', unsavedFile='', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=[], opt=off} execute: Command{action=Command::SetUnsaved, file='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T//irony-server-0895e5', unsavedFile='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T/irony-unsaved-HJLctH', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=[], opt=off} execute: Command{action=Command::Complete, file='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T//irony-server-0895e5', unsavedFile='', dir='', line=1, column=2, prefix='', caseStyle='exact', flags=['-x', 'c++'], opt=off} execute: Command{action=Command::Candidates, file='', unsavedFile='', dir='', line=0, column=0, prefix='inc', caseStyle='case-insensitive', flags=[], opt=off} execute: Command{action=Command::SetUnsaved, file='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T//irony-server-0895e5', unsavedFile='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T/irony-unsaved-rzrZZT', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=[], opt=off} execute: Command{action=Command::Parse, file='/var/folders/qd/dqf8z3ss0_l118qsqyrm81140000gn/T//irony-server-0895e5', unsavedFile='', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=['-x', 'c++'], opt=off} execute: Command{action=Command::Diagnostics, file='', unsavedFile='', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=[], opt=off} execute: Command{action=Command::ResetUnsaved, file='/tmp/abcd.cpp', unsavedFile='', dir='', line=0, column=0, prefix='', caseStyle='exact', flags=[], opt=off}

My Analysis

When a buffer is not backed by a file on disk, irony--buffer-state-compare returns set-unsaved, and irony--unsaved-buffers-tasks will send set-unsaved to the server, using - as the file name. Kind of a hack here to make sure nothing is really done, since there is no file anyway.

But the first time it is saved, the new state says the file is now unmodified, but was modified before. So the condition in irony--buffer-state-compare falls into the last case, and sends a reset-unsaved to the server. The server tries to remove the item from a map, but finding none (of course since the file was just created), hence the error.

I added the check to make sure that only when old file exists, and the file changed from modified to unmodified, do we send reset-unsaved to the server.

I'm not sure if I misunderstood the semantics of unsaved and broke some other stuff though.

hotpxl commented 6 years ago

Maybe related to #408 ?

Sarcasm commented 6 years ago

Hello, thanks for the detailed explanation and solution. Reading your description and the code, I agree with the fix.

I'm wondering if the reset-unsaved should be done old-state's file, instead of relying on the new buffer's file, but I cannot come up with a problematic situation.

hotpxl commented 6 years ago

I think the question is, while old-state and new-state both have a file field, they must be identical right? I'm not sure if moving a file in dired will cause any discrepancy.

Sarcasm commented 6 years ago

I don't think they are necessarily identical. I think, you can do things like C-x -Cw, aka write-file, to target a new file for example.

set-visited-file-name seems to be a setter for a buffer file.