Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
and definitions. It also does this for Rizin annotations, such as RZ_OWN. This works
by defining the RZ_BINDINGS preprocessor flag, which sets the annotations to expand to
__attribute__((annotate)), which can be picked up by libclang.

Additionally, the linter checks for missing ownership annotations (RZ_OWN or RZ_BORROW)
on functions that return pointer types requiring ownership tracking (e.g., RzList,
RzPVector, RzGraph, etc.). This helps ensure proper memory management documentation.
"""

from typing import List, Dict, Set, TypedDict, Optional, cast
Expand All @@ -42,6 +46,7 @@
CursorKind,
SourceRange,
SourceLocation,
TypeKind,
)


Expand Down Expand Up @@ -78,6 +83,23 @@ def cursor_get_annotations(cursor: Cursor) -> List[str]:

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.

"RzList",
"RzListIter",
"RzPVector",
"RzVector",
"RzGraph",
"RBTree",
"SdbList",
"HtPP",
"HtUP",
"HtUU",
"HtPU",
"HtSP",
"HtSS",
"HtSU",
}


def cursor_get_comment(cursor: Cursor, *, packed: bool = False) -> Optional[str]:
"""
Expand Down Expand Up @@ -185,6 +207,31 @@ def cursor_get_comment(cursor: Cursor, *, packed: bool = False) -> Optional[str]
return comment


def cursor_returns_pointer_type(cursor: Cursor) -> Optional[str]:
"""
Check if a function cursor returns a pointer to a type that requires ownership annotation.

Returns the type name if it does, None otherwise.
"""
if cursor.kind != CursorKind.FUNCTION_DECL:
return None

return_type = cursor.result_type
if return_type.kind != TypeKind.POINTER:
return None

for child in cursor.get_children():
if child.kind == CursorKind.PARM_DECL:
break

if child.kind == CursorKind.TYPE_REF:
type_name = child.spelling
if type_name in ownership_required_types:
return type_name

return None


@dataclass
class Arg:
"""
Expand Down Expand Up @@ -213,6 +260,18 @@ def __init__(self, cursor: Cursor):
self.annotations = cursor_get_annotations(cursor)
self.comment = cursor_get_comment(cursor)

returned_type = cursor_returns_pointer_type(cursor)
if returned_type:
has_ownership = any(
ann in self.annotations for ann in ["RZ_OWN", "RZ_BORROW"]
)
if not has_ownership:
warn(
f"Missing ownership annotation (RZ_OWN or RZ_BORROW) at "
f"{self.location} for function '{self.name}' "
f"returning {returned_type} pointer"
)

self.args = [
Arg(
annotations=cursor_get_annotations(arg), comment=cursor_get_comment(arg)
Expand Down
Loading