syntastic closes an open location list on Write

[chrisbra]

Hi, I just noticed, that synastic closes an already open location list. It would be nice, if when no error is found, syntastic would reopen the last open location list (e.g. by using :lolder or something a like).

Thanks.

[lcd047]

No errors to display --> no loclist --> no error window. :) Coming up with an older list just to keep the window there doesn't make much sense as far as I can tell.

[chrisbra]

That is wrong. If I have created a location list by e.g. using lvimgrep then syntastic shouldn't close it, just because there is no error when compiling that file. It should simply leave my location list alone.

[lcd047]

Ah, I see. I don't think there is any easy way to achieve that. The way we handle opening and closing the error window is already pretty fragile.

[chrisbra]

Here is a patch, that works for me. Please consider merging https://github.com/chrisbra/syntastic/commit/b8eb1d27dd92449ebf7278567fc64f4c02883387

[lcd047]

That breaks in two ways: it opens the error window even if it was previous empty, and it populates the loclist with the wrong set of locations if there are errors, but g:syntastic_always_populate_loc_list and g:syntastic_auto_jump are both 0. There is also a more subtle problem: calling setloclist() after getloclist() can lose information, since syntastic-created loclists have non-standard attributes along with the standard ones. As I said, it isn't that obvious.

[lcd047]

One more thing: if all errors are fixed and the previous list of locations was generated by Syntastic, you don't want to resurrect the errors as zombies. I don't see how you could distinguish between pre-existing loclist entries and Syntastic-created ones based only on getloclist().

[lcd047]

The second patch addresses only the first problem I noted above, opening the error window even if it's empty. It doesn't seem to address the other ones.

[chrisbra]

Indeed. I still find it disappointing, that this breaks several plugins, that create a location list for themselves. There must be a simpler way for syntastic to not mess with location lists, that have not been created by syntastic.

[chrisbra]

I don't mind, having to create a new location list if syntax errors are found, but if not, it should still leave the old location list alone

[lcd047]

I agree, Syntastic should be able to coexist peacefully with pre-existing loclists. There might be a way to achieve that (we already do it for signs), but as far as I can tell it wouldn't be completely trivial.

[lcd047]

This is probably a step in the right direction: 003751a.

[scrooloose]

Just so I understand, here is what is happening right?

1. Open a file with no errors
2. Add a loclist --> :call setloclist(0, [{'text': "example", 'lnum': 10, 'bufnr': bufnr("")}])
3. :lopen
4. :write

This causes the location list that was opened in step 3 to be closed by this: https://github.com/scrooloose/syntastic/blob/master/plugin/syntastic/autoloclist.vim#L33

This is a known problem that I have been pondering for a while. I have not found a nice solution and I'm pretty sure there isnt one.

Ideally we want to be able to tag an entire loclist (or its individual entries) as being created by syntastic. Then we would be able to tell if we were messing with someone else's loclist.

One hacky way could be to set one of the less critical items in each loclist item to a magic value. e.g. set the nr attribute to 99 so we know we created it. Then we could scan the loclist before closing it and ensure we dont close someone else's. Or we could change our approach and simply add/remove syntastic items to the existing loclist.

Im just floating the idea really, thoughts?

[lcd047]

@scrooloose: I think you essentially nailed it. The bigger issue is dealing in a meaningful way with loclists created by other plugins (or perhaps lvimgrep). Once this is solved, it should be trivial to decide when to close or open the loclist window.

As I see it, there are three possible approaches to dealing with the main problem:

1. Make :lolder and :lnewer work with syntastic. A step towards that was my patch 003751a above, but that turned out to be problematic (see #641), and Vim makes it next to impossible to control exactly what loclist is active at any given time.
2. Use "mixed" loclists, that is, make syntastic add / remove errors to the old loclist. This can be messy, since we use non-standard attributes for loclist errors. It can also be confusing to the user (personally I wouldn't want syntax errors mixed with, say, grep results in a single list).
3. Save the previous loclist, let syntastic do its thing, then restore the old loclist when done. The main problem here is being able to distinguish syntastic loclists from external loclists.

Of these, 3. seems the least problematic approach. Inventing a magic attribute for tagging is really ugly, but it should work. I thought about this for a while, and I haven't been able to find anything better.

[chrisbra]

Hi Martin!

Would it help, if we create a patch of the Vim source code, that adds an additional parameter to the setqflist()/setloclist() functions, which would set the w:quickfix_title variable accordingly?

This means, syntastic would call setloclist(0, [....], '', 'syntastic locationlist') to create the locationlist and whenever it needs to work with the quickfix list, it can simply query the w:quickfix_title variable to find out, if this location list has been created by syntastic.

(I have a patch ready, it just needs to be submitted to Bram for inclusion. It might however take a while until Bram commits it, so this isn't a solution, that will be available soon).

regards, Christian

[lcd047]

@chrisbra: I'm not Martin, but I'd say such a patch would be useful but not enough. First, because we would still have to come up with some sensible behaviour for older versions of Vim, and second, because given a buffer, there is no way (at least not an obvious one) to find out which window corresponds to the associated loclist. Unlike quickfix windows, there can be multiple loclist windows.

If you're willing to go that way though, I'd really suggest taking your time to fix the real problem: managing error lists from VimL. I'd suggest at least these new features:

• functions to read and edit the stack of saved error lists
• functions to get the current position in that stack, and to move the stack pointer (like :lolder and :lnewer)
• a function to get the owner window of an error list
• options for :grep and friends to create a new error list or reuse an existing one (right now it always reuses an existing list)
• options for :make and friends to create a new error list or reuse an existing one (right now it always creates a new list)
• an option to add a tag to an error list, which could be used, as you suggest, for setting w:quickfix_title.

As far as I can tell, none of these should be particularly hard to implement. The functionality is already there, it just has to be exported to VimL. In my opinion it would make a huge difference: it would actually make error lists manageable from scripts. With the current interface, trying to do anything beyond the basics with error lists is pretty much hopeless.

[lcd047]

Ok, tagging would be something like this: a18a04b. The "tag" error is marked automatically as invalid, but that doesn't prevent it from being displayed. So this is not a viable idea. :(

[chrisbra]

Hi LCD!

I created some patches:

1. set w:quickfix_title with setqflist(): https://groups.google.com/d/msg/vim_dev/X7VVPd4Do5s/7j0TX6KJOp0J
2. read stack of error lists (and get current position in error stack): https://groups.google.com/d/msg/vim_dev/51EmHJW6oSA/z556jnB0LLEJ (write is not necessary, can be done by using a combination of lolder/setqflist...)
3. Get tdoesn't make much sense, to get the owner of an error list, because window numbers are not persistent they change too fast.
4. an option to add/create new error list for make/grep etc. I am not sure, this makes much sense and I am afraid, this does not provide enough value to have it accepted (so I haven't created a patch for that yet).

regards, Christian

[lcd047]

@chrisbra: That's great, thank you so much!

The patch that allows setting w:quickfix_title from setqflist() looks good, there isn't much to be said about it.

The patch adding getlocstack() and setlocstack() seems to work as intended, but I'm not sure about the interface. What if the stack gets extended to 15 items, then to 20? I'd suggest a simpler approach: just make getlocstack() return a list of loclists, make a separate getlocstackptr() return the index (1-based, to follow the VimL convention) of the current pointer into this list, then add a separate function to return the title of the loclist for a given index. A setlocstackptr() counterpart to getlocstackptr() would indeed be redundant, but it would still be nice to have, for symmetry. That would make a much cleaner interface in my opinion.

Getting the owner window of an error list is important, because without it there is no safe way to solve problems like the one described in #400. Window numbers being ephemeral should not be a problem, since VimL is single-threaded. In my opinion setloclist() taking a window argument is a design flaw, but we're stuck with it by now.

As for an option for :make and friends (not) to create a new loclist, that's what it takes to make :lolder and :lnewer work properly. Have you noticed that plugins using :grep (such as Ack, Ag, and many others) work fine with these functions, but none of the plugins using the :make mechanism do? Why do you think that is?

[chrisbra]

Hi LCD!

On So, 12 Mai 2013, LCD 47 wrote:

@chrisbra: That's great, thank you so much!

The patch that allows setting w:quickfix_title from setqflist() looks good, there isn't much to be said about it.

The patch adding getlocstack() and setlocstack() seems to work as intended, but I'm not sure about the interface. What if the stack gets extended to 15 items, then to 20?

Doesn't matter. It will still work correctly.

I'd suggest a simpler approach: just make getlocstack() return a list of loclists, make a separate getlocstackptr() return the index (1-based, to follow the VimL convention) of the current pointer into this list, then add a separate function to return the title of the loclist for a given index. A setlocstackptr() counterpart to getlocstackptr() would indeed be redundant, but it would still be nice to have, for symmetry. That would make a much cleaner interface in my opinion.

Hm, I don't really mind. Please raise your voice in the mentioned vim_dev thread. I just want to know, what others think about this. If we generally agree on that, it would be great and I'll implement it but I just want to make sure other developers have the chance to comment on this.

Getting the owner window of an error list is important, because without it there is no safe way to solve problems like the one described in #400. Window numbers being ephemeral should not be a problem, since VimL is single-threaded. In my opinion setloclist() taking a window argument is a design flaw, but we're stuck with it by now.

The api isn't really well defined for this. The window number is just a counter going through the list of windows in the vim source. So returning the window number for a location list doesn't help much. Between the call of getlocstack() and restoring the stack, the windows could have changed already (think of autocommands). I am not sure how to approach this.

As for an option for :make and friends (not) to create a new loclist, that's what it takes to make :lolder and :lnewer work properly. Have you noticed that plugins using :grep (such as Ack, Ag, and many others) work fine with these functions, but none of the plugins using the :make mechanism do? Why do you think that is?

I have no clue as I generally tend to not use many plugins besides my own.

So how would you suggest to specify such an option? We can't use ! as this attribute is already taken. I can only think of adding yet another option or creating another command. But this issue should probably also be raised at vim_dev.

regards,

Christian

[chrisbra]

Hi LCD!

As far as I can see, :grep and :make work exactly the same (only [l]grepadd adds to an existing quickfix list all :make and :grep should always create a new quickfix list). So what is really the problem with :make and why doesn't it happen with :grep? Do you know about any more detail (tickets/discussions) about this issue?

regards, Christian

[lcd047]

@chrisbra: Sure, I'll post a note to vim_dev about getlocstack().

Re: windows numbers: you're right, there is an even deeper design problem there, but I don't think it has any play in what I'm referring to. Basically, I'm asking for some way to convert between:

1. the current window
2. the associated loclist, when applicable
3. the associated loclist window, when applicable.

Ephemeral window IDs should be fine for that. A possible problem might come up related to "orphaned" loclists, but that could be signaled by returning -1 for a window ID. Not being able to tell which loclist window belong to which buffer is a really annoying situation. :)

As for the :make and :grep, I'm referring to lines 2858-2862 in src/quickfix.c in the current HEAD:

res = qf_init(wp, fname, (eap->cmdidx != CMD_make
                         && eap->cmdidx != CMD_lmake) ? p_gefm : p_efm,
                                                  (eap->cmdidx != CMD_grepadd
                                                  && eap->cmdidx != CMD_lgrepadd),
                                                  *eap->cmdlinep);

The fourth parameter to qf_init() is newlist. I'm not aware of any discussion about it, since I stopped following the Vim lists some ~7 years ago (I re-subscribed recently). But I did some testing on my own a few days ago, related to this very issue, and the conclusion was :grep-based plugins have a somewhat easier life. :)

[chrisbra]

Hi LCD!

On Mo, 13 Mai 2013, LCD 47 wrote:

@chrisbra: Sure, I'll post a note to vim_dev about getlocstack().

Re: windows numbers: you're right, there is an even deeper design problem there, but I don't think it has any play in what I'm referring to. Basically, I'm asking for some way to convert between:

1. the current window
2. the associated loclist, when applicable
3. the associated loclist window, when applicable.

Ephemeral window IDs should be fine for that. A possible problem might come up related to "orphaned" loclists, but that could be signaled by returning -1 for a window ID. Not being able to tell which loclist window belong to which buffer is a really annoying situation. :)

That sounds useful. I am just not sure how to accomplish that. I need to think about it.

As for the :make and :grep, I'm referring to lines 2858-2862 in the current HEAD:

res = qf_init(wp, fname, (eap->cmdidx != CMD_make
                         && eap->cmdidx != CMD_lmake) ? p_gefm : p_efm,
                                                  (eap->cmdidx != CMD_grepadd
                                                  && eap->cmdidx != CMD_lgrepadd),
                                                  *eap->cmdlinep);

The fourth parameter to qf_init() is newlist.
Yeah exactly. And If you read this correctly, it means except for (l)greapadd, Vim will always create a new location list/quickfix list.

Whatever problems some plugins have, needs to be detailed and is not a problem for this issue here currently.

regards,

Christian

[lcd047]

@chrisbra: In that particular context, "except for (l)greapadd" translates to "(l)make" -- which is what I was claiming.

Basically, (l)make creates a stack of loclists (each invocation adds a new one on top), while (l)grep creates only one loclist (subsequent invocations reuse it). Consequently, running :lolder after a series of (l)grep gets you to whatever loclist was there before you started, and life is good, while running :lolder after a series of (l)make makes you go through the stack, which sucks. With (l)make there is no way to go directly to the loclist that was in place before you started, no way to tell where in the stack you are (not from VimL anyway), and even if you're careful to count your steps, there is an opportunity to intermix other things in the run chain which will throw off your count.

Now, from the point of view of usability, both behaviours (last shot vs. stack) make sense, which is why I'm suggesting a global option to control it.

[lcd047]

Oh, I see: (l)grep is not the same as (l)grepadd, and there is no such thing as (l)makeadd. Sorry about that.

[lcd047]

I committed a new branch loclist_count, that makes syntastic reuse loclists whenever it's reasonably safe to do it. This doesn't solve the initial problem, but it does make :lolder and :lnewer more useful in relation to syntastic.

Please note that this only works with Vim 7.4 or later. A relevant bug has been fixed in one of the last pre-releases of 7.4 (the bug could make Vim segfault when loclists are reused).

[blueyed]

@lcd047 Has the loclist_count branch been merged / used? (it does not exist anymore) Is it https://github.com/scrooloose/syntastic/tree/loclist_state?

[lcd047]

@blueyed: Yes, it's controlled by g:syntastic_reuse_loc_lists. But it's off by default (and undocumented) because of #1127. I never got to the bottom of that problem either.

[lorin]

Note that this issue breaks a vim-go feature: https://github.com/fatih/vim-go/issues/814

[lcd047]

@lorin Please see #1650 instead.