Skip to content

Add bufnr to prop_list() and prop_find()#8647

Closed
arp242 wants to merge 2 commits intovim:masterfrom
arp242:tprop-bufnr
Closed

Add bufnr to prop_list() and prop_find()#8647
arp242 wants to merge 2 commits intovim:masterfrom
arp242:tprop-bufnr

Conversation

@arp242
Copy link
Contributor

@arp242 arp242 commented Jul 27, 2021

Getting properties with prop_list() don't return a buffer number:

:echo prop_list(line('.'))[0]
{'id': 0, 'col': 2, 'end': 1, 'type': 'keyword', 'length': 5, 'start': 1}

Getting this property fails as it's defined for this buffer:

:echo prop_type_get('keyword')
{}

So I need to do:

:echo prop_type_get('keyword', #{bufnr: bufnr('')})
{'highlight': 'Keyword', 'end_incl': 0, 'start_incl': 0, 'priority': 0, 'bufnr': 1, 'combine': 1}

But it's not clear from the prop_list() return value that this is the
case: I need to try both.

The same issue exists with prop_find():

:echo prop_find(#{type: 'keyword'})
{'lnum': 10, 'id': 0, 'col': 2, 'end': 1, 'type': 'keyword', 'length': 5, 'start': 1}

This fixes the issue by adding the type_bufnr to the return value of
both functions. I used type_bufnr because I thought that was a lot
clearer than just bufnr, as "bufnr" can refer to either the bufnr of the
property or of the property type (adding the bufnr of the property
itself in the return value doesn't make much sense, but it's easy to get
the two mixed up).

There is still a bit of an issue with this as this will fail:

    # E158: Invalid buffer name: 0
    var props = prop_list(line('.'))
    echo props->mapnew((_, v) => prop_type_get(v.type, {bufnr: v.type_bufnr}))

You need to do:

    echo props->mapnew((_, v) => prop_type_get(v.type, (v.type_bufnr ? {bufnr: v.type_bufnr} : {})))

Not sure what the best fix for that would be; on one hand you want to
get an error if you pass an invalid bufnr, but on the other hand being
able to get the prop types from the prop_list() output without a lot of
if/else checks. Maybe just leaving it as-is is fine.

Getting properties with prop_list() don't return a buffer number:

	:echo prop_list(line('.'))[0]
	{'id': 0, 'col': 2, 'end': 1, 'type': 'keyword', 'length': 5, 'start': 1}

Getting this property fails as it's defined for this buffer:

	:echo prop_type_get('keyword')
	{}

So I need to do:

	:echo prop_type_get('keyword', #{bufnr: bufnr('')})
	{'highlight': 'Keyword', 'end_incl': 0, 'start_incl': 0, 'priority': 0, 'bufnr': 1, 'combine': 1}

But it's not clear from the prop_list() return value that this is the
case: I need to try both.

The same issue exists with prop_find():

	:echo prop_find(#{type: 'keyword'})
	{'lnum': 10, 'id': 0, 'col': 2, 'end': 1, 'type': 'keyword', 'length': 5, 'start': 1}

This fixes the issue by adding the type_bufnr to the return value of
both functions. I used type_bufnr because I thought that was a lot
clearer than just bufnr, as "bufnr" can refer to either the bufnr of the
property or of the property type (adding the bufnr of the property
itself in the return value doesn't make much sense, but it's easy to get
the two mixed up).

There is still a bit of an issue with this as this will fail:

        # E158: Invalid buffer name: 0
        var props = prop_list(line('.'))
        echo props->mapnew((_, v) => prop_type_get(v.type, {bufnr: v.type_bufnr}))

You need to do:

        echo props->mapnew((_, v) => prop_type_get(v.type, (v.type_bufnr ? {bufnr: v.type_bufnr} : {})))

Not sure what the best fix for that would be; on one hand you *want* to
get an error if you pass an invalid bufnr, but on the other hand being
able to get the prop types from the prop_list() output without a lot of
if/else checks. Maybe just leaving it as-is is fine.
@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #8647 (92a0802) into master (53ba05b) will decrease coverage by 0.05%.
The diff coverage is 97.59%.

❗ Current head 92a0802 differs from pull request most recent head 2f5030c. Consider uploading reports for the commit 2f5030c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8647      +/-   ##
==========================================
- Coverage   90.08%   90.02%   -0.06%     
==========================================
  Files         150      146       -4     
  Lines      169497   169176     -321     
==========================================
- Hits       152686   152302     -384     
- Misses      16811    16874      +63     
Flag Coverage Δ
huge-clang-none 89.22% <97.64%> (+0.10%) ⬆️
huge-gcc-none 89.58% <96.01%> (+<0.01%) ⬆️
huge-gcc-testgui 88.22% <97.58%> (+0.03%) ⬆️
huge-gcc-unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/version.c 92.34% <ø> (ø)
src/digraph.c 93.22% <50.00%> (-1.12%) ⬇️
src/misc2.c 92.28% <57.14%> (-0.15%) ⬇️
src/typval.c 94.28% <88.88%> (-0.98%) ⬇️
src/usercmd.c 94.70% <91.66%> (-0.10%) ⬇️
src/clientserver.c 88.38% <94.44%> (+0.07%) ⬆️
src/filepath.c 90.52% <97.61%> (+0.03%) ⬆️
src/evalfunc.c 96.37% <97.84%> (-0.03%) ⬇️
src/arglist.c 91.70% <100.00%> (+0.17%) ⬆️
src/change.c 92.99% <100.00%> (+0.02%) ⬆️
... and 80 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ba05b...2f5030c. Read the comment docs.

@lacygoill
Copy link

You need to do:
echo props->mapnew((_, v) => prop_type_get(v.type, (v.type_bufnr ? {bufnr: v.type_bufnr} : {})))

Actually, this would not work all the time, because we can't use a number as a boolean. So, v.type_bufnr might give an error at compile time, or at runtime. It will definitely give one at runtime if v.type_bufnr evaluates to a non-boolean number (i.e. any number other than 0 and 1).

So, we need:

echo props->mapnew((_, v) => prop_type_get(v.type, (v.type_bufnr != 0 ? {bufnr: v.type_bufnr} : {})))
                                                                 ^--^

As for the issue, I'm not sure to have fully understood, but couldn't we make prop_type_get() handle 0 specially? That is, {bufnr: 0} would be the same as {}. We would still get an error for other invalid buffer numbers, like {bufnr: -1}.

@arp242
Copy link
Contributor Author

arp242 commented Jul 27, 2021

Yeah, that example is slightly off and only works for bufnr=1; I discovered that later as well when I used it, just didn't update this.

couldn't we make prop_type_get() handle 0 specially? That is, {bufnr: 0} would be the same as {}. We would still get an error for other invalid buffer numbers, like {bufnr: -1}.

Yeah, could do that; problem is that if you accidentally pass 0 you won't get an error. A case could be made for both behaviours; I figured I'd just put it here and see what folks think.

@lacygoill
Copy link

lacygoill commented Jul 27, 2021

prop_type_get() handling 0 specially would not be an exception. arglistid() uses 0 to identify the global arglist.

problem is that if you accidentally pass 0 you won't get an error.

A similar issue exists with bufexists() and bufname(). When they receive 0, they interpret it as meaning the alternate buffer for the current window. Which might cause an issue with a function like getqflist(). The latter uses 0 with a different meaning: it's used for an entry with a non-existing buffer. The help recommends to explicitly check for 0:

(Note: some functions accept buffer number zero for the alternate buffer,
you may need to explicitly check for zero).

@brammool
Copy link
Contributor

brammool commented Jul 27, 2021 via email

@arp242
Copy link
Contributor Author

arp242 commented Jul 28, 2021

Also, I would not know why you would accidentally have a zero buffer number.

Was mostly thinking about erroneous user input, logic errors, stuff like that.

At any rate, I changed it now.

@brammool brammool closed this in e2390c7 Jul 28, 2021
@arp242 arp242 deleted the tprop-bufnr branch July 28, 2021 13:21
chrisbra pushed a commit to chrisbra/vim that referenced this pull request Aug 30, 2021
Problem:    prop_list() and prop_find() do not indicate the buffer for the
            used type.
Solution:   Add "type_bufnr" to the results. (closes vim#8647)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants