[bhuvan-somisetty]

Description

Fixes indexing, assignment, id handling, and equality in grass.pygrass.vector.basic:

1. Bbox.__eq__ compares coordinates (nsewtb()) and returns NotImplemented for other types. __hash__ = None is set explicitly because the coordinates are mutable.
2. BoxList.__setitem__ assigns bbox.c_bbox.contents (the C struct) instead of the Python wrapper, which failed with TypeError: expected bound_box instance, got Bbox.
3. BoxList.__getitem__ / __setitem__ share one index helper: operator.index() (so numpy integers work), negative indices, bounds checks (the box array is a raw C pointer), and GrassError for lists that hold only ids. Slices are supported in __getitem__.
4. BoxList.append uses len(self) as the new id like before, but falls back to max(ids) + 1 when that id is taken, so append after remove() or on query ids no longer silently drops the box. It raises GrassError if the box was not added, and copies the box first, so views from boxlist[i] are not read after the C array is reallocated.
5. BoxList.extend (previously commented out) keeps the ids of another BoxList, so feature ids from spatial queries stay valid, and raises ValueError on colliding ids. Other iterables get new ids and are validated and copied before anything is appended.
6. BoxList.remove is documented as removing by id, not position, and accepts any integer type.
7. Removed a leftover pdb comment in Ilist.__getitem__.

Motivation and context

Fixes #8046.

How has this been tested?

• python/grass/pygrass/vector/tests/test_basic.py: 42 tests pass, including with MALLOC_PERTURB_ set. The tests for each issue raised in review fail on the original code.
• Doctests in basic.py pass, except CatsList, which crashes the same way on main and is unrelated.
• ruff check (including preview rules on the changed lines) and ruff format --check.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as before)

Checklist

• PR title provides summary of the changes and starts with one of the pre-defined prefixes
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[echoix]

And also the following possible findings by AI at the end of this details block, that are about the behavior of remove() and the fact that it uses the id.

All these shortcomings should have a test case to show it fails now and is fixed later

Details

1. Why extend was commented out

It was never live. It came in already commented out, with # FIXME: doesn't work, in the very first pygrass import (0982f55fa6, Pietro Zambelli, 2012-10-10). No later commit touched it.

Why it didn't work: the commented code called the C function Vect_boxlist_append_boxlist, which calls Vect_boxlist_append for each box. Vect_boxlist_append (lib/vector/Vlib/list.c:293) silently skips any id that is already in the list. Python's BoxList.append uses len(self) as the id, so every BoxList built in Python has ids 0..n-1. Appending one such list to another only produces duplicate ids, and nothing gets added. I ran that call on two 2-box lists and the length stayed 2 instead of 4.

The PR's plain Python loop gets around this by giving each box a new id.

2. Linter exclusions for this file

pyproject.toml has no per-file ignores for basic.py, so there is nothing file-specific to re-enable. Two project-wide settings do matter here:

• PLW1641 (eq-without-hash) is ignored globally (pyproject.toml:275). That is the only reason ruff accepts the new Bbox.__eq__. Running ruff with that rule on flags basic.py:15.
• ERA001 (commented-out code) isn't selected at all. On main it would have flagged the dead extend block. On the PR it still flags basic.py:330, a leftover # import pdb; pdb.set_trace() in Ilist.__getitem__.

The missing-docstring rules (D1) are also ignored globally.

3. Why __setitem__ changed so much

The actual fix is one line. The rest is defensive checking, and most of that checking is justified.

• It never worked. The 2012 original had a typo (self.c_boxlist.cotents...). Markus Metz fixed the typo the next day (9ae2701257), but the line still assigned the Python Bbox wrapper into a C array. ctypes rejects that: TypeError: expected struct_bound_box instance, got Bbox. The fix is to assign bbox.c_bbox.contents, the underlying C struct, which ctypes copies.
• The bounds checks protect memory. box is a raw C pointer (LP_struct_bound_box), not a ctypes array, so indexing it is unchecked. On a 1-element list, box[5].N returned 0.0 from past the end of the buffer instead of raising. A negative index would read before the buffer. So the wrap-around and range checks are needed.
• Where your suspicion holds:
  • The same normalise-and-check block is copied in __getitem__ and __setitem__. One small helper would be cleaner.
  • isinstance(indx, int) now rejects numpy integers: bl[np.int64(0)] raises TypeError. Using operator.index() would accept them.

4. Code review

/superpowers:requesting-code-review isn't installed in this session, so I ran the built-in code-review skill on the PR commit. Each finding below was checked on the build.

Should be fixed or discussed on the PR:

1. Boxes silently dropped after remove. BoxList([a, b, c]).remove(0) leaves ids [1, 2]. A following append(d) tries id 2, which already exists, so the C function skips it and returns 0. The length stays 2, and the new GrassError never fires: the C function only returns 1 for a NULL list. The test that mocks a return value of 1 covers a path that can't happen in real use.
2. remove works by id, not position. Calling remove(0) twice removes only one box. The docstring says "index value". The PR's test passes only because the ids happen to equal positions, and its final remove(bl_other) has no assertion.
3. extend loses ids. Spatial query results (e.g. find_by_bbox.areas(..., bboxlist_only=True)) carry feature ids. extend renumbers them as len, len+1, …, so those ids no longer point to the right features.
4. Use-after-free in bl.extend(bl). When the C array has to grow, Vect_boxlist_append reallocates it before copying from a pointer into the old buffer. With 1000 boxes and MALLOC_PERTURB_ set, the first copied box came out as garbage (-2.49e-127). The same applies to any Bbox taken from bl[i] before the list grows.
5. Bbox is now unhashable. Adding __eq__ without __hash__ makes hash(Bbox()) raise TypeError, so code that puts boxes in a set or uses them as dict keys breaks. This is what the ignored PLW1641 rule would catch. Also, __eq__ should return NotImplemented rather than False for non-Bbox values.

Minor:

• The numpy-integer index rejection from section 3.
• Lists that hold only ids (have_boxes=0) have a NULL box pointer that indexing doesn't check for.
• The pdb leftover from section 2.

Items 1–3 come from the C design that ties ids to length, and they predate the PR. The PR's new error handling doesn't actually catch them, though. A deeper fix would use a unique id (e.g. max(ids)+1) and avoid passing in pointers that come from the same list.

I can post these as a review on the PR, or draft suggested patches, if you want.

Details

The PR’s plain Python loop gets around this by giving each box a new id.
Does this cause any problems for equivalence checks, or when the same bbox is touched by C code?

I'll check where ids are actually read, in pygrass and in the C library, and then test the cases that matter on the build.

I found that pygrass reads bboxlist.ids to fetch features (vector/init.py:833,923; find.py:595). Next I'll check which C functions take a boxlist as input and whether the select functions reset the list they fill.

The select functions reset the box list before filling it, so they never read existing ids—only Vect_boxlist_append_boxlist, Vect_boxlist_delete_boxlist, and Vect_val_in_boxlist actually read ids from an input list. I'll now test this on a real vector map in a temporary project.

Results are in. One more case: extending a query result, whose ids aren't 0..n-1.

Equality checks are fine. The problems show up in anything that reads the ids, and pygrass and the C list functions both do. I tested each point below on the dev build, using a 3×3 grid from v.mkgrid in a temporary project. A bounding-box query on that grid returned area ids [5, 6, 8, 9].

Equality checks: no problem

• Bbox.__eq__ only compares coordinates. A Bbox has no id, so renumbering can't affect it. Every box in an extended copy compared equal to its source.
• BoxList has no __eq__. Two lists with the same boxes are never equal (copy == found gave False), whatever their ids. That was already true before the PR.

Where the new ids do cause problems

The id isn't a label pygrass makes up: query results use it to store the feature id. pygrass reads it to fetch features (for f_id in bboxlist.ids at vector/__init__.py:833 and :923, and find.py:595). Three C list functions also look boxes up by id: Vect_boxlist_append_boxlist, Vect_boxlist_delete_boxlist and Vect_val_in_boxlist.

1. The ids stop pointing at the right features. After copy.extend(found), the ids were [0, 1, 2, 3] instead of [5, 6, 8, 9]. Looking up area categories by those ids gave [-1, 1, 2, 3]: id 0 triggered GRASS's "invalid id" warning, and ids 1–3 silently returned the wrong areas.
2. Id-based operations between the two lists stop matching. Vect_val_in_boxlist(copy, 5) returned 0, and copy.remove(found) removed nothing (the length stayed 4).
3. Boxes get dropped silently. Extending a query result with two new boxes gives the first one id 4 (the current length). The second one gets id 5, which already belongs to a query box, so the C code skips it. The ids ended up as [5, 6, 8, 9, 4] and the length was 5 instead of 6. No error is raised. This is the same skip as the after-remove case from my first review.

The C select functions (Vect_select_*_by_box) don't care about any of this. They call Vect_reset_boxlist and refill the list from scratch, so they never read existing ids.

When C code works on the same box

• Appending stores a copy. Vect_boxlist_append copies the C struct, so changing the original Bbox afterwards leaves the list alone (north stayed 1.0 after I set it to 99).
• bl[i] returns a view into the C array, not a copy. Writing to it changes the list (setting north = 42 showed up in bl[0]). That view is only valid until the array grows: the next append that reallocates frees the old buffer and leaves the view pointing at freed memory. It is the same mechanism as the bl.extend(bl) use-after-free I reproduced earlier. The PR doesn't add this hazard, but by making extend usable it makes it easier to hit.

What this means for the PR

Renumbering is what makes extend work at all for lists built in Python, whose ids are always 0..n-1. Keeping the source ids (Vect_boxlist_append_boxlist) instead would drop those boxes as duplicates, which is the original FIXME. So neither option is right in every case.

A cleaner design would be:

• keep the source ids when the input is a BoxList, as the commented-out code did;
• give new boxes max(ids) + 1 instead of len(self);
• raise an error when the C code skips a box as a duplicate, instead of silently losing it.

At a minimum, the extend docstring should say that ids are reassigned, so nobody passes a query result through it and then reads features by id.

[bhuvan-somisetty]

Thanks for the thorough review @echoix! Pushed fixes for all of it in 81fbd7d, and each issue now has a test that fails on the old code. extend was dead since the first pygrass import because of duplicate ids, so now it keeps BoxList ids and raises on collisions. Also switched to operator.index() for numpy ints, and subclasses pass isinstance fine. Renamed b1/bl too :)

[echoix]

I won't reread tonight, but you know what would be nice? To have about the same kind of tests, but for the C side only of the API, not in relation to pygrass. Some library code tests have been merged in the not too distant past, so something similar.

It could probably help showing that the C code works without the python code protecting too many situations

[bhuvan-somisetty]

Good idea! Added them in 4e1c6fe, calling the C functions directly with no pygrass. They make it pretty clear the C side quietly skips duplicate ids and deletes by id, which is where most of the pygrass issues came from. No rush on the reread :)

[echoix]

Good idea! Added them in 4e1c6fe, calling the C functions directly with no pygrass. They make it pretty clear the C side quietly skips duplicate ids and deletes by id, which is where most of the pygrass issues came from. No rush on the reread :)

I didn't think of doing it right away here, but sure. If there's problems to fix though, maybe a different pr to get a review from a more appropriate reviewer for C code. Otherwise it's a recipe to have a stalled PR, where no one can be confident to review all pieces of a PR.

[echoix]

Note to self: I can't find the comment from the email, but about the new ids being max(ids)+1, to check about the run time (big O notation concept). It might be O(n^2).

[bhuvan-somisetty]

Makes sense! These only test what the C code does today, no C changes here. If anything needs fixing on that side I'll open a separate PR for it.

[bhuvan-somisetty]

Good call, it actually was O(n^2). Building 20k boxes took ~50s vs 0.1s on main. Now it tries len() as the id first like before and only falls back to max+1 if that id is taken, so it's back to main's speed.