Skip to content

Add linter warnings for missing ownership annotations#90

Open
Ayushd785 wants to merge 6 commits intorizinorg:devfrom
Ayushd785:add-ownership-annotation-warnings
Open

Add linter warnings for missing ownership annotations#90
Ayushd785 wants to merge 6 commits intorizinorg:devfrom
Ayushd785:add-ownership-annotation-warnings

Conversation

@Ayushd785
Copy link
Contributor

@Ayushd785 Ayushd785 commented Jan 7, 2026

description

adds a linter warning for functions that return ownership-sensitive pointer types (RzList, RzPVector, RzGraph) without an explicit ownership annotation.

When a function returns one of these types as a pointer and is missing RZ_OWN or RZ_BORROW, the linter now emits a warning indicating the function and location.

ScreenShot
image

Fixes
#34

src/lint.py Outdated

generic_types = {"RzList", "RzListIter", "RzPVector", "RzVector", "RzGraph"}

ownership_required_types = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the change. But why only these types? Any pointer which is not const needs ownership info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yaaa makes sense, this should apply to all non-const pointers. I'll update the logic to check if the return type is a non-const pointer instead of checking against specific type names. Will it be a good approach then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const merely means that the data structure and/or pointer is read-only ... the caller might still need to free the returned const pointer depending on whether the annotation is RZ_OWN or RZ_BORROW

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so you're saying even const pointers need ownership annotations since const only affects mutability, not ownership?
But then, should we check ALL pointer returns, or still focus on specific types like container types (RzList, RzVector, etc.) that are heap-allocated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const merely means that the data structure and/or pointer is read-only

i disagree. i expect any const pointer (returned) to be always borrowed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i expect any const pointer (returned) to be always borrowed

That is a reasonable expectation and should be coded in the linter logic, meaning that there should be a warning when RZ_OWN is used with a returned const pointer.

Hmm, still need to check all pointer returns.

Anyway, I'm ok with checking all non-const pointer returns as a start. Hopefully there won't be a ton of warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, lets do all non-const pointers. I assume the amount of changes would be significant. We can run linter manually first before merging, fix all occurences, and then merge the PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning that there should be a warning when RZ_OWN is used with a returned const pointer.

Good idea! Second this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! So i have to check ALL non-const pointer returns for missing ownership annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, everyone, for being inactive here. I am on a very tight schedule for the next 3-4 days. I will implement the suggested changes as soon as i get some time.

@Ayushd785 Ayushd785 force-pushed the add-ownership-annotation-warnings branch from baa6b3f to 9262149 Compare January 30, 2026 22:08
@notxvilka
Copy link
Contributor

image

It works! up for any feedback Thank you!

You will need excluding the subprojects/ that are external (like pcre2 on your screen) and not provided by Rizin itself (like rzheap).

@Ayushd785
Copy link
Contributor Author

image It works! up for any feedback Thank you!

You will need excluding the subprojects/ that are external (like pcre2 on your screen) and not provided by Rizin itself (like rzheap).

Okay i'll do it

@Ayushd785
Copy link
Contributor Author

Ayushd785 commented Jan 31, 2026

image It works! up for any feedback Thank you!

You will need excluding the subprojects/ that are external (like pcre2 on your screen) and not provided by Rizin itself (like rzheap).

Ahhh! The bug was here

for command in commands:
            abspath = os.path.abspath(
                os.path.join(command["directory"], command["file"])
            )
            relpath = os.path.relpath(abspath, rizin_path)

            if relpath.startswith("subproject") or relpath.startswith("test"):
                continue

There is a bug it should be subprojects not subproject. That's why the check is ignored. I'll fix that

@Ayushd785
Copy link
Contributor Author

Ayushd785 commented Jan 31, 2026

hi @notxvilka, I've added an extra check in check_translation_unit() to
filter out external subproject headers

if "/subprojects/" in abspath:
            subproject_name = abspath.split("/subprojects/")[1].split("/")[0]
            if not subproject_name.startswith("rz"):
                continue

This assumes anything in subprojects/ that doesn't start with "rz" is an
external library and skips the linter check on it.

Also fixed that minor typo.
I am up for any feedback. Thank you!

@Ayushd785
Copy link
Contributor Author

Ayushd785 commented Jan 31, 2026

wait I have missed something. I need a logic which includes everything which has initials as 'rz' but does not have an underScore with it (eg. rz_util) i thought it was just rzheap

edit - fixed

@Ayushd785 Ayushd785 force-pushed the add-ownership-annotation-warnings branch from 1483b91 to 6919138 Compare January 31, 2026 19:20
@Ayushd785
Copy link
Contributor Author

Screencast.From.2026-02-01.00-55-12.mp4

@notxvilka
Copy link
Contributor

Ok, this is ready now. But before merging this, we need to fix those warnings in Rizin first, so that CI will not become instantly red.

@Ayushd785
Copy link
Contributor Author

Ok, this is ready now. But before merging this, we need to fix those warnings in Rizin first, so that CI will not become instantly red.

Okay Sure.

@Ayushd785
Copy link
Contributor Author

Ayushd785 commented Feb 1, 2026

Ok, this is ready now. But before merging this, we need to fix those warnings in Rizin first, so that CI will not become instantly red.

Well, I was just wondering. what approach are you planning to use to fix
those thousands of linter warnings? I am just curious to know how you gonna automate it.?

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. But yeah, fixing all the warnings is needs some smart idea.

@notxvilka
Copy link
Contributor

Created the issue to track this: rizinorg/rizin#5870

@Rot127
Copy link
Member

Rot127 commented Feb 3, 2026

What about we add a file in our repo root which contains the modules to check?

In the beginning it contains only modules which don't throw any warnings.
Then, we can fix up each module, and add its name in the file so rz-bindgen checks it?

Otherwise I don't see us making any progress?

Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>
@Ayushd785
Copy link
Contributor Author

What about we add a file in our repo root which contains the modules to check?

In the beginning it contains only modules which don't throw any warnings. Then, we can fix up each module, and add its name in the file so rz-bindgen checks it?

Otherwise I don't see us making any progress?

This would be a great approach! CI stays green while we make progress. In addition to this We could also add a script that auto-detects which modules are already clean (0 warnings) and suggests adding them to the whitelist. I'd be happy to implement this if it's useful!

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.

5 participants