kovisoft / slimv

Official mirror of Slimv versions released on vim.org
455 stars 60 forks source link

Find-Definition broken in a few ways #49

Open phmarek opened 7 years ago

phmarek commented 7 years ago

The Find-Definition is broken in a few ways.

Perhaps https://github.com/phmarek/slimv/commit/854ca0dd5866d7ebe59da8b83c7b6d7fd89a219e helps a bit here.

Thanks for integrating that into SLIMV!!

kovisoft commented 7 years ago

Hi Phil,

Thank you for the report and for the code sample, I'll try to check the issue(s) in the weekend.

Best regards and have a nice weekend!

/Tamás

On Thu, Jan 12, 2017 at 4:55 PM, Philipp Marek notifications@github.com wrote:

The Find-Definition is broken in a few ways.

  • It doesn't take the current package at the cursor position into account
  • For methods, swank will return multiple results - for the DEFGENERIC and the DEFMETHOD forms. This result is just ignored; it should be displayed in a new buffer, so that the user can choose. (Like the debugger or inspect windows).
  • Don't use the tags file; swank returns a file and a position already, so just jump to that one. The tags file doesn't work in many cases (#+NIL comes to my mind).

Perhaps phmarek@854ca0d https://github.com/phmarek/slimv/commit/854ca0dd5866d7ebe59da8b83c7b6d7fd89a219e helps a bit here.

Thanks for integrating that into SLIMV!!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kovisoft/slimv/issues/49, or mute the thread https://github.com/notifications/unsubscribe-auth/AD9LQsH8dajCD1H2YzwCfUayyvou1CXgks5rRkzcgaJpZM4Lh6TQ .

kovisoft commented 7 years ago
phmarek commented 7 years ago

Thank you very much for your help!

As for the third point, the tags file is incomplete. It won't have any information about functions that got autogenerated - via Lisp macros, via DEFCLASS (accessors - reader/writer functions), etc.

So I believe that the first try should be done via the (byte or other) position that's returned by swank; and that one might well list multiple choices, again eg. with accessor methods, so even if the local tags file only has one result (perhaps the DEFGENERIC) it would most probably be the wrong one.

kovisoft commented 7 years ago

Maybe there is a misunderstanding here. The tags file (actually a termporary tags file) is generated on the fly from the information returned by swank for the swank:find-definitions-for-emacs message. So it will be as complete as any other implementation using the same swank method. The tags file is just a means to handle the tag stack.

phmarek commented 7 years ago

Oh, I see. Yeah, that should work fine. I'm sorry for the misunderstanding.

Nonetheless, I have quite some problems jumping to accessor methods, macro-generated functions, global variables, etc., although a wireshark trace clearly shows that information being sent from the Lisp process to slimv... Perhaps there's some parsing problem left?

kovisoft commented 7 years ago

Yes, it's possible that there are some parsing problems, so I'd like to ask you to report any such situations, and if possible then please include a code sample that I can use to reproduce/investigate the problem. Thank you.

phmarek commented 7 years ago

Hmmm, after going to your HEAD commit, I can't reproduce the problems right now.

Perhaps they're already solved.... I'm sorry about the noise.

Thank you very much!

phmarek commented 7 years ago

Some other problem occurred after updating to 011e282038f5ee6c41c4a3ffa3ba380fda48923a; for names not found within the inferior lisp I get

Fehler beim Ausführen von "function SlimvFindDefinitionsPrompt[4]..SlimvFindDefinitionsForEmacs":
Zeile   10:                                                                                                                                                        
E433: Keine Tag-Datei
E426: Konnte Tag "atomic-hash-indizes" nicht finden
Fehler beim Ausführen von "function SlimvFindDefinitionsPrompt":
Zeile    4:
E171: Fehlendes :endif

I guess E171 ("missing endif") is because the try/catch exception handling means that the closing endif within the try block isn't seen, and then reported.

Touching a tags file in the current directory restores the wanted behaviour - I only get the "not found" message.

Sorry about the german messages.

kovisoft commented 7 years ago

(No problem, I understand some German :))

I was unable to reproduce the problem on my system, but I think the missing endif' problem can be solved by replacing the catch statement in function SlimvFindDefinitionsForEmacs() with a more general one, like this:

    catch /.*/

Could you please check it? Thank you.

phmarek commented 7 years ago

Hmmm, you mean just replacing? Just asking, because I'm already catching E73 in another clause as well...

And, to the best of my understanding, this E171 is thrown when (neo)vim encounters the :endfunction, ie. after the try/catch clause... Yes, this would be more like a bug in neovim, perhaps I should report there.

kovisoft commented 7 years ago

I don't know how valid is throwing E171 at that point, but I think the preceding E433 is the origin of the problem. Right now it is unhandled but we should really handle it the same way as E426. So I was just thinking that why not just catch all exceptions and display the "not found" message.

I was able to reproduce the E171 by modifying the slimv code (catching another exception number instead of E426), so I don't think this is a neovim bug, rather it's the way VimL works (I don't know the details, just guessing).

kovisoft commented 7 years ago

I modified the exception handling in commit https://github.com/kovisoft/slimv/commit/cc2ba8e4ce5f37c276c2ad8a823650f714a94fa7 as described above. Please check if it solves this issue. Thank you.

phmarek commented 7 years ago

The exception handling looks good so far...

But I still need a (locally generated!) tags file. If I remove it, no definitions at all work; if I touch it, swank will return only one match (and nvim will jump to that one); only if I run ctags, all occurrences are found and listed.

It works better if the string that gets passed to swank:find-definitions-for-emacs includes the package name, ie. is "package:name" instead of only name.

With my simple change of using SlimvFindPackage, if used within a defpackage form, the wrong package is being selected.

(defpackage :foo
  (:use :cl)
  (:export #:bar

With the cursor on #:bar, I'd expect foo:bar to be used for the find-definition query. But that's not what happens, the earlier in-package form is found. (Yeah, this is not a regression, but just another usage problem.) Yes, I replace "#:":

diff --git i/ftplugin/slimv.vim w/ftplugin/slimv.vim
index e0ba3d9..63cc0f7 100644
--- i/ftplugin/slimv.vim
+++ w/ftplugin/slimv.vim
@@ -1163,7 +1163,8 @@ function! SlimvSelectSymbol()
     endif
     let symbol = expand('<cword>')
     call winrestview( oldpos )
-    return symbol
+    let symbol2 = substitute(symbol, '^#:', '', '')
+    return symbol2
 endfunction

 " Select symbol with possible prefixes under cursor and return it
@@ -3446,7 +3447,8 @@ function! SlimvFindDefinitionsPrompt()
     if SlimvConnectSwank()
         "let symbol = input( 'Find Definitions For: ', SlimvSelectSymbol() )
         let symbol = SlimvSelectSymbol()
-        call SlimvFindDefinitionsForEmacs( symbol )
+        call SlimvFindPackage()
+        call SlimvFindDefinitionsForEmacs( s:swank_package . "::" . symbol )
     endif
 endfunction

Still, slimv shows only :go <position> in the tags list, instead of the :snippet that swank provides; this makes navigation much harder. The ctags generated file shows better information.

Thank you for your help!

kovisoft commented 7 years ago

Sorry, I'm a bit lost here. I can't really decide how many problems are there and what exactly they are.

phmarek commented 7 years ago

Do you use a dedicated tags file definition (g:slimv_tags_file)? Normally a new tempfile is generated for each session to be used by SlimvFindDefinitionsForEmacs(), but in this case you can't just touch it beforehand.

I had one in my checkout. When reading your new code, I removed it - but then the FindDefinition stopped working. So I have to have a tags file - but it can be empty.

Do you also use a dedicated tags file generated by ctags?

As long as I don't get all answers from swank, yes ;)

Do you actually report a bug in SlimvFindPackage()?

Well, I'm not sure whether this special case belongs to SlimvFindPackage or into FindDefinition.

SlimvFindDefinitionsPrompt() reads the symbol name from the prompt. Why and where should it search for a package, when there is no (or not always a) connection between the symbol entered and the cursor location?

Well, in absence of a better default, why not choose the package the cursor is at? Furthermore, I changed the default to take the symbol under the cursor, so that matches my usecase perfectly.

Do you have an issue with the tag list display?

Yes. swank typically returns a snippet of the source code, which makes deciding which place to jump to much easier. The tags file from ctags shows the source line, too.

With the current implementation slimv only shows me the position - "go 482", which isn't that helpful when there are multiple matches.

Thank you very much for your patience, help, and time!

kovisoft commented 7 years ago

Ah, now I see your problem with the tags file (or at least I hope so). As far as I know the swank server's swank:find-definitions-for-emacs function requires that the source must be compiled, otherwise it can't find the definition and returns this or similar: (:error "Error: DEFINITION-SOURCE of function MYFUNC did not contain meaningful information.") as it can be seen in the swank.log file (if you enable logging). This does not depend on whether the (temporary) tags file exists or not, because the problem here is that without compilation swank really does not return any location information.

phmarek commented 7 years ago

No, I'm sorry, of course the files need to be compiled. Here's an example:

The query:

(:emacs-rex (swank:find-definitions-for-emacs "regex-helper::re-resolve-reg-name") "regex-helper" :repl-thread 396)

The answer from swank is this (reformatted, snippets heavily abbreviated):

(:return 
 (:ok 
  (("(DEFINE-COMPILER-MACRO RE-RESOLVE-REG-NAME)" 
    (:location (:file ".../regex-helper.lisp") 
               (:position 5770) 
               (:snippet "(define-compiler-macro re-resolve-reg-name (&whole whole c-regex reg-name)")))
   ("(DEFUN RE-RESOLVE-REG-NAME)" 
    (:location (:file ".../regex-helper.lisp") 
               (:position 5537) 
               (:snippet "(defun re-resolve-reg-name (c-regex reg-name)")))
   ("(DECLAIM RE-RESOLVE-REG-NAME
   INLINE)" 
    (:location (:file ".../regex-helper.lisp") 
               (:position 2628) 
               (:snippet "(declaim  "))))) 396)

But the location list looks like

   # pri verw. tag                                Datei
  1 F C      regex-helper::re-resolve-reg-name  .../regex-helper.lisp                                            
               :go 5770
  2 F C      regex-helper::re-resolve-reg-name  .../regex-helper.lisp
               :go 5537
  3 F C      regex-helper::re-resolve-reg-name  .../regex-helper.lisp
               :go 2628

In contrast, when using ctags, the result looks like this:

   # pri verw. tag                Datei
  1 F C f    re-resolve-reg-name  regex-helper.lisp
               (define-compiler-macro re-resolve-reg-name (&whole whole
  2 F C f    re-resolve-reg-name  regex-helper.lisp
               (defun re-resolve-reg-name (c-regex reg-name)

which displays much more useful information than the file-positions (":go").

phmarek commented 7 years ago

Oh, and BTW, when trying to find something with more than one location, I came across this:

(:emacs-rex (swank:find-definitions-for-emacs "common-lisp-user::print-object") ":common-lisp-user" :repl-thread 391)

My nvim reports:

Fehler beim Ausführen von "function SlimvFindDefinitionsPrompt[5]..SlimvFindDefinitionsForEmacs[4]..SlimvCommandGetResponse[11]..provider#python3#Call":
Zeile   18:
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "~/.config/nvim//bundle/slimv/ftplugin/swank.py", line 1347, in swank_output
    result = swank_listen()
  File "~/.config/nvim//bundle/slimv/ftplugin/swank.py", line 850, in swank_listen
    myitems = [[elem[1][1][1], elem[1][2][1]] for elem in params]
  File "~/.config/nvim//bundle/slimv/ftplugin/swank.py", line 850, in <listcomp>
    myitems = [[elem[1][1][1], elem[1][2][1]] for elem in params]
IndexError: list index out of range

Most probably because the swank result contains errors:

("(DEFMETHOD PRINT-OBJECT T T)"
 (:ERROR "Error: The value NIL is not of type NUMBER"))
("(DEFMETHOD PRINT-OBJECT :AFTER SB-EXT:FINAL-DEPRECATION-WARNING T)"
 (:LOCATION (:FILE "sbcl/src/code/condition.lisp")
  (:POSITION 68009) (:SNIPPET ...)))
("(DEFMETHOD PRINT-OBJECT SB-EXT:SIMD-PACK T)"
 (:ERROR "Error: The value NIL is not of type NUMBER"))
("(DEFMETHOD PRINT-OBJECT SB-KERNEL:FDEFN T)"
 (:LOCATION (:FILE "sbcl/src/code/print.lisp")
  (:POSITION 21052) (:SNIPPET ...)))

But it would be nice if these were caught (and ignored?).

kovisoft commented 7 years ago

Then again, I don't understand what your exact problems are. Sometimes you write tags just don't work for you, then you get error results, another time you get normal results with formatting problems. There are just too many issues reported here with no clear (for me) problem descriptions. I'm trying to take them one by one, but I can't really fix things if I don't know what they are and when they happen. I'd like to ask you to file separate issues for each separate problems, or at least we should try to take them here one by one.

(1) tags don't work Please, tell me what is your relevant slimv tags settings, please give me a code sample and tell me what you do to it and what happens. Also if possible then please copy here the related swank.log messages. I really don't see how could the swank result depend on the tags file being already created or not.

phmarek commented 7 years ago

With no ./tags file:

weiter in function SlimvFindDefinitionsPrompt[3]..SlimvFindDefinitionsForEmacs

Zeile 5:     endif
Zeile 6:     try
Zeile 7:         if msg != ''
Zeile 8:             exec ":tjump " . msg
Zeile 9:         else
Zeile 10:             exec ":tjump " . a:symbol
Zeile 10: :tjump atomic-hash
Ausnahme geworfen: Vim(tjump):E433: Keine Tag-Datei

With a zero-byte ./tags file:

Zeile 5:     endif
Zeile 6:     try
Zeile 7:         if msg != ''
Zeile 8:             exec ":tjump " . msg
Zeile 9:         else
Zeile 10:             exec ":tjump " . a:symbol
Zeile 10: :tjump atomic-hash
Tag-Datei tags wird durchsucht
Ausnahme geworfen: Vim(tjump):E426: Konnte Tag "atomic-hash" nicht finden

With ctags -R I get the expected result:

Find Definitions For: atomic-hash   # pri verw. tagDatei
  1 F C f    atomic-hash       atomic-unique-hash.lisp                                                                                                                                            
               (defclass atomic-hash ()
  2 F   f    atomic-hash       package.lisp
               (defpackage atomic-hash

Buffer (or nvim globally) has this set:

tags=/tmp/nvimAsd1TI/1,./tags;,tags

Complete log coming via email.

phmarek commented 7 years ago

Sorry about the confusion, BTW.

kovisoft commented 7 years ago

Thanks for the logs, but do you also have the swank.log file that contains the communication between slimv and the swank server? In the first two cases I see no major difference, even if the resulting vim exceptions are different. The temporary tags file was not created because the swank server did not return any useful location information (that's why I would like to have the swank.log file). In the first case there was no tags file and it was not created upon find-definitions, so vim complained about the missing tags file. In the second case there was an empty tags file, but it was not filled by find-definitions, so vim complained about the missing tag definition. In both cases the real problem is that swank did not return location information (or slimv was unable to parse it correctly). So it does not really matter whether you created the temporary tags file manually or not. Could you please send me the swank communication logs too? Thank you.

kovisoft commented 7 years ago

Thanks for the logs, I copy the relevant part here:

(:return (:ok (("(DEFCLASS ATOMIC-HASH)" 
(:error "Error: DEFINITION-SOURCE of class ATOMIC-HASH did not contain meaningful information.")) 
("(DEFPACKAGE ATOMIC-HASH)" (:location (:file "/home/no_backup/MINE/lisp-learn/lognav-2016/package.lisp") (:position 482) (:snippet "(defpackage atomic-hash
  (:use :cl)
  (:export #:atomic-hash
           #:make-atomic-hash
           #:atomic-hash-done
           #:atomic-hash-insert
           #:atomic-hash-canonical
           #:atomic-hash-aref
           #:atomic-hash-gethash
    "))))) 5)

It seems that find-definitions returned multiple locations, one of them however contained an error and this broke the result parsing. I fixed the parsing in commit https://github.com/kovisoft/slimv/commit/9ed03de89c084a7fdc89800f3d1422c74cf327d2 but I cannot do much about what the swank server returns. Anyway, I hope this fix solves the tags file errors you got.

phmarek commented 7 years ago

Thanks a lot, that commit avoids the error.

May I now ask for point 2 - Show (parts of) the :SNIPPET instead of :GO nnn, to actually see what I'd jump to for each tag?

kovisoft commented 7 years ago

Normally ctags uses the code snippets as search commands (the editor searches for the text between the two slash characters). However the swank server primarily uses the :position value (this is the byte offset from the beginning of the file) for returning the location, and the snippet may not be returned at all, like in the below example:

(:ok (("#'MAKE-SEQUENCE" 
(:location (:file "c:/ccl/lib/sequences.lisp") (:position 3274) nil)) ("(COMPILER-MACRO MAKE-SEQUENCE)" 
(:location (:file "c:/ccl/compiler/optimizers.lisp") (:position 74597) nil)))) 8)

Moreover the "search by snippet" approach may fail if the symbol name contains special characters that confuse the regular expression matcher. Therefore I'd like to keep the location based on the :position value. One solution to that can be to append the snippet at end of the :go nnn command as a comment, like this (here the double quote character starts a VimL comment):

  1 F C      test              test.lisp 
               :go 72 " (defun test (x) (* x x))

What do you think about this solution?

phmarek commented 7 years ago

Oh, the :go is VimL command that gets executed? I wondered about that already....

Yes, appending (the first line) of a snippet or, if there is none, the string outside the :LOCATION etc. tags ("#'MAKE-SEQUENCE" resp."(COMPILER-MACRO MAKE-SEQUENCE)" in your example above) would be a nice way to show where the tag leads to.

kovisoft commented 7 years ago

That's a good idea, I implemented it in commit https://github.com/kovisoft/slimv/commit/0ba36cf1c5c41b5d536fab58cdd809a8a368300e , please check it. Thank you.

Note on the tags file format: A tag definition can contain one of these:

phmarek commented 7 years ago

With this change I get

Vim(tjump):E431: Falsches Format in Tag-Datei "/tmp/nvimc1DMy2/1"

because my swank produces multi-line snippets - and inserting these into the tags file breaks the expected layout.

BTW, is there a good reason for that parenthesis?

#!/usr/bin/env python)
kovisoft commented 7 years ago

Then again please give me some more information, e.g. the swank communication log, the contents of the tags file.

phmarek commented 7 years ago

But you already got a swank.log, and even cited from it in https://github.com/kovisoft/slimv/issues/49#issuecomment-294743861... there's a multi-line snippet, even.

kovisoft commented 7 years ago

Yes, it's possible but there was so much noise in this thread, so much unclear (for me) and confusing (for me) information ... ;)

OK, I'm checking it.

phmarek commented 7 years ago

Thank you!

Next time I'll try to make my issues easier to read and understand.

kovisoft commented 7 years ago

OK, next iteration comes in commit https://github.com/kovisoft/slimv/commit/44c32e5b5658521f5dddc899ef65fcbac754c1fc .

phmarek commented 7 years ago

Thanks, that doesn't give any error.

One or two wishes I'd like to leave here, though...

kovisoft commented 7 years ago
phmarek commented 7 years ago
kovisoft commented 7 years ago

Yeah, it should be two lines per tag. But with long snippets, the comment line flows into the next screen line.

Again, this is how vim's default tag lookup works. If you create a tags file using ctags then the long lines will be flowing into the next screen line.

phmarek commented 7 years ago

Yeah. But when creating the tags file, why not cut lines so that they don't need multiple screen lines?

kovisoft commented 7 years ago

OK, done, see commit https://github.com/kovisoft/slimv/commit/3fde9e773451b17112d0201686d6c7be1a5b4972 .