[vim/vim] quickfix: no efficient way to get error type counts (PR #20434)
yilisharcs
Problem: There is no efficient way to get the count of each error type
in a quickfix list. Retrieving this information requires
iterating over the entire list in script, which is O(N).
Solution: Maintain a type-frequency histogram in 'qf_list_T' and expose
it via the 'total' key in getqflist() and getloclist().
(yilisharcs)
Co-authored-by: Gemini 3 Flash
Note: neovim/neovim#40110
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20434
Commit Summary
- 13aa7a4 quickfix: no efficient way to get error type counts
File Changes
(2 files)
- M runtime/doc/quickfix.txt (15)
- M src/quickfix.c (84)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20434)Do you think this PR is ready for review?
If you feel it's not quite there yet, I would highly recommend converting it to a Draft PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
yilisharcs
yilisharcs left a comment (vim/vim#20434)i don't know how else to improve this. and i did spend a reasonable amount of time on it. all that's left is for people with actual experience with the vim internals to have a look. i promise i did not vibecode this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20434)Thanks for the work and the explanation — and no concern about how it was
written; the points below are purely about the code, and they should give you
concrete things to improve.
The feature is useful, but there are a few correctness issues and a design
question that should be resolved before this is ready for review. None of them
require deep internals knowledge — they're mostly about keeping the cached
histogram in sync with qf_type.
1. Multiline 'errorformat' makes the counts wrong
qf_add_entry() increments the histogram with the entry's type at insertion
time, but for multiline formats the type is filled in afterwards in
qf_parse_multiline_pfx() (quickfix.c around line 1470):
if (vim_isprintc(fields->type) && !qfprev->qf_type) qfprev->qf_type = fields->type;
The entry was already counted under its initial type (usually 0), so the
later overwrite to e.g. 'E' is never reflected. The histogram ends up
counting such entries under the empty-string key instead of their real type.
2. Copying a location list loses the types
qf_copy_list_entries() calls qf_add_entry(..., 0, ...) (type 0) and then
overwrites the type right after (quickfix.c around line 2625):
prevp->qf_type = from_qfp->qf_type; // error type
So every entry in a copied list is counted under type 0. This path runs when
a window with a location list is split, so getloclist(w, {'total': 1}) after
a split returns wrong counts.
Both #1 and #2 are the classic hazard of an incrementally maintained cache:
every place that mutates qf_type now has to update the histogram too, and
two such places were missed. Worth considering whether computing the counts
on demand (iterate the list only when 'total' is requested) is simpler and
removes this whole class of bug. 'total' is a rare query, so the one-time
O(N) walk is unlikely to matter in practice.
3. :helpgrep entries produce a control-character key
qf_type == 1 is used for :helpgrep. The display side treats both 0 and
1 as "no type" (qf_types() returns "" for both). But qf_getprop_total()
only maps i == 0 to the empty-string key; i == 1 becomes a dict key
consisting of the single byte 0x01. That's inconsistent with the displayed
type and surprising for callers. Consider folding 1 into the empty key as
well.
4. Only valid entries are counted — sum won't match size
The increment is guarded by if (qfp->qf_valid), while size counts all
entries including invalid ones. So the sum of the total values does not equal
size. That may be intentional, but the documentation only says "count of each
error type" and doesn't state it. Please either document "valid entries only"
or count all entries so the two agree.
5. Tests
There are no tests in this PR. Each of the issues above is easy to turn into a
regression test (a multiline 'errorformat' case, a location-list split case,
a :helpgrep case, and a basic count check). Adding them would also have
surfaced #1–#3.
6. API design: the key name total
total reads like a single number, and there's already a size key for the
entry count. What this returns is a per-type histogram, so a name like
typecount would communicate the shape better. This is an API name that can't
be changed later, so it's worth settling now.
Given the above (in particular #1, #2 and the missing tests), I'd still suggest
converting this to a Draft until they're addressed. Happy to look again once
they are.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
yilisharcs
yilisharcs left a comment (vim/vim#20434)
- Tests
There are no tests in this PR. Each of the issues above is easy to turn into a
regression test (a multiline 'errorformat' case, a location-list split case,
a :helpgrep case, and a basic count check). Adding them would also have
so, funny thing... i would have made tests/ran the test suite if trying to build vim from source (with no changes!) and set $VIMRUNTIME didn't cause it to crash with a buffer overflow on launch. i'm pretty sure this is something about my nixos environment, but i had hoped i could sidestep this for now...
regarding all others, thanks! i will act on your advice
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
yilisharcs
yilisharcs left a comment (vim/vim#20434)1 & 2. currently looking into it
Worth considering whether computing the counts
on demand (iterate the list only when 'total' is requested) is simpler and
removes this whole class of bug. 'total' is a rare query, so the one-time
O(N) walk is unlikely to matter in practice.
i'm making a (neovim) plugin that uses async jobs and the quickfix list to make something like emacs' compilation mode. it currently has this loop--
local items = vim.fn.getqflist({ lines = batch, efm = ctx.efm }).items for _, item in ipairs(items) do if item.valid == 1 then local t = (item.type and item.type ~= "") and item.type:upper() or "I" local key = ctx.counts[t] and t or "I" ctx.counts[key] = ctx.counts[key] + 1 end end
--to keep a live count of the valid entries on the statusline. it's not rare for me in the sense that i was testing it with ripgrep (which was slow to stream the data into the quickfix list, but that's a whole another PR) on my home directory, so that naive loop would be called thousands of times per second. now assuming i haven't misunderstood how neovim interfaces with this structure, for the data to get from the core to my plugin i first have to call getqflist() to parse the quickfix list and produce a vimscript dictionary, which is then parsed again to be converted into a lua table, and finally be used to increment the counters. that's two passes and lots of allocations, right? i thought that i didn't have a skill issue and that this could be remedied by exposing this data directly from the core, so here i am.
-
ok.
-
that's on purpose! i updated the docs to reflect that.
-
no problems on this front either (now that i fixed the damn compiler)
-
i opted to go with "typecount" as you suggested.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20434)I have also a few concerns. the name was already mentioned, but I am a bit afraid of the additional memory costs this incurs. This makes 1024 bytes for each quickfix list and we have 10 by default + additional locationlists (depending on 'chistory' setting). That is much when we probably only have a handful of different types (E, W, N, I).
And somewhat related I am also wondering if we really need to populate the counters by default, instead of only when this is explicitly asked for. But I am not sure this will work for your usecase?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.