Discard quickfix errors when quickfix doesn't locate a file to jump to

[athuth]

Tries to fix #287 by dropping from the QuickFix list any items that don't locate existing files.

Known issue: If cc 1 jumps to a new buffer immediately, the open-buffer message "{filename}" {linenumber}L, {colnumber}C hides the messages that tell the user about the dropped errors. Haven't been able to figure out how to get around that. The messages are just a little "extra" anyway.

Please review when able and let me know if you find any other issues.

[fatih]

Hi @athuth

Thanks for the PR. Can you please give more detail what was wrong and what this is now fixing for us? I don't know if it's a good idea to check all existing files if they exist or not. This could slow down the overall performance.

[athuth]

Hey @fatih - here's a playground snippet that will show the behavior this is trying to fix with :GoRun.

Say you're working on a project and you want to show some trace/info/warning lines at runtime, something important enough to print but not important enough to save to a file. Unless your project has only one or two files, you probably pass log.Lshortfile or log.Llongfile. The compiler errorformat picks those up as errors and then tries to open files with names like "INFO 23:00:00 main.go", which of course don't exist, but QuickFix puts them in unlisted buffers anyway and shows them in the qflist. If the first item is one of these, :GoRun even jumps back into a blank file that doesn't exist, annoying and unhelpful.

Using the log package is not the only way you could encounter this, but it's a pretty straightforward case of a feature of the language (well, I should say, of the standard library) that doesn't play nice with :GoRun.

As far as performance impact, I admit I haven't looked at it. Maybe someone who has a large Go project can try it out and see if it makes a noticeable difference; I'm just starting to get into Go so I have nothing big to test it on, and didn't notice any change with only a few items in the QuickFix list.

Important to keep in mind, it only checks files that show up in the QuickFix list, which normally would not be every file in the project. And QuickFix has to process each of these items, and create unlisted buffers for all the files it finds, currently, so if you drop some items from the qflist you save some processing by QuickFix.

I could be better about avoiding redundant calls to filereadable(), if it's an expensive call. For example:

     let qflist = getqflist()
     let errors = []
-    let dropped = {}  " use dict to mimic a set/avoid duplicates
+    let seen = {}
     for item in qflist
         let filename = bufname(item.bufnr)
-        if filereadable(filename)
+        if !(get(seen, filename) && get(errors, item)) && filereadable(filename)
             call add(errors, item)
+            let seen[filename] = 'added'
         else
-            let dropped[filename] = ''
+            let seen[filename] = 'dropped'
         endif
     endfor
-    for k in keys(dropped)
+    for k in keys(filter(seen, 'v:val == "dropped"'))
         echo "GoRun: Dropped" k "from QuickFix list (file not readable in working directory)"
     endfor
     call setqflist(errors)

That might be a smarter approach, but it's a bit less straightforward and I don't know how it would affect performance, or if it even needs to be optimized.

[fatih]

Hi @athuth

Thanks for tackling this issue, much appreciated! I have two things in mind:

1. I think we can solve this entirely by leveraging it to the parser section in compiler/go.vim file. There are a lot details that could detect non files and not include them. Did you check it with :help errorformat ?
2. I you have only one file and have ten errors, your current implementation (not the one in your comment) will check the same file ten times. As you said, it needs to changed and improved. Can you add the fixes in your comment to the final PR ?

[athuth]

@fatih Both good points.

1. I had the same thought at first but it seems impossible to predict exactly which messages/patterns are errors. I expect some false positives are unavoidable. Have you checked to make sure the first commit of this PR correctly documents the goals of the existing patterns?

   It might be possible to improve or add to the errorformat patterns in a way that plays more nicely with the log package. I'll look into it when I have an opportunity.

2. I have pushed a commit to avoid multiple redundant checks. A little simpler and better implementation than the example I suggested above.

[fatih]

You need to drop the : at the end, it's not a valid syntax.

[fatih]

Thanks @athuth

I've pulled and test it. There was an error with the : character, once I've removed it everything worked perfect. Can you please fix the remaining comments and squash all your changes into one commit.

I think this is a wonderful contribution. Thank you that you put your time here, much appreciated 👍

[athuth]

@fatih That's what I get for being lazy and testing changes in the installed plugin, then typing them directly into the GitHub web interface, instead of just cloning the branch locally to begin with. Lesson learned.

Fixed and squashed. I'm happy to be able to contribute.

[fatih]

Thanks @athuth, merged 👍

[mrnugget]

Thanks for this @athuth! And thanks to @fatih for merging :)