Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a tool for stubs in cstruct v4 #72

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Create a tool for stubs in cstruct v4 #72

wants to merge 11 commits into from

Conversation

Miauwkeru
Copy link
Contributor

@Miauwkeru Miauwkeru commented Feb 27, 2024

The only thing that does not work as intended yet is the dereference of pointer. As in, it doesn't return the desired type information but just T

@Miauwkeru Miauwkeru changed the base branch from main to refactor-v4 February 27, 2024 10:13
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 58.38509% with 67 lines in your changes missing coverage. Please review.

Project coverage is 89.65%. Comparing base (7f6a584) to head (20a00bd).

Files Patch % Lines
dissect/cstruct/tools/stubify.py 38.31% 66 Missing ⚠️
dissect/cstruct/types/base.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   92.04%   89.65%   -2.39%     
==========================================
  Files          20       21       +1     
  Lines        2136     2292     +156     
==========================================
+ Hits         1966     2055      +89     
- Misses        170      237      +67     
Flag Coverage Δ
unittests 89.65% <58.38%> (-2.39%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Schamper Schamper self-requested a review March 4, 2024 00:38
@Miauwkeru Miauwkeru marked this pull request as ready for review March 11, 2024 12:50
@Miauwkeru Miauwkeru marked this pull request as draft March 11, 2024 12:50
Base automatically changed from refactor-v4 to main May 17, 2024 13:15
@Miauwkeru Miauwkeru marked this pull request as ready for review August 9, 2024 14:56
dissect/cstruct/tools/stubify.py Show resolved Hide resolved
dissect/cstruct/tools/stubify.py Outdated Show resolved Hide resolved
dissect/cstruct/tools/stubify.py Outdated Show resolved Hide resolved
dissect/cstruct/tools/stubify.py Outdated Show resolved Hide resolved
dissect/cstruct/tools/stubify.py Outdated Show resolved Hide resolved
dissect/cstruct/types/structure.py Outdated Show resolved Hide resolved
dissect/cstruct/types/structure.py Outdated Show resolved Hide resolved
dissect/cstruct/cstruct.py Outdated Show resolved Hide resolved
dissect/cstruct/cstruct.py Outdated Show resolved Hide resolved
def _type_stub(cls, name: str = "") -> str:
return f"{name}: {cls.__name__}"

def to_stub(cls, name: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name this differently? Also I think this might as well be a private method. Maybe _to_type_stub? to_stub might be confusing because it also makes me think of the compiler.

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 renamed it to to_type_stub as it is a function that gets called externally.

Copy link
Member

Choose a reason for hiding this comment

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

I don't agree on the "externally" part, I'd classify this usage as internal and would never expect anyone to ever call this externally. This is not part of any expected usage API.

description = """
Create stub files for cstruct definitions.

These stub files are in a `.pyi` format and provides `type` information to cstruct definitions.
Copy link
Member

Choose a reason for hiding this comment

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

Why is type monospaced? Also this would need to be double back ticks anyway, if you're going to include this on the dissect.cstruct doc page.

Comment on lines +124 to +125
This in turn gives a developer insight into the elements inside the definition and
parameter completion when dealing with cstructs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This in turn gives a developer insight into the elements inside the definition and
parameter completion when dealing with cstructs.


prev_offset = buffer.tell()
for const, value in c_structure.consts.items():
buffer.write(indent(f"{const}: {type(value).__name__}=...\n", prefix=indentation))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buffer.write(indent(f"{const}: {type(value).__name__}=...\n", prefix=indentation))
buffer.write(indent(f"{const}: {type(value).__name__} = ...\n", prefix=indentation))

Copy link
Member

Choose a reason for hiding this comment

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

Can you add some unit tests for this file? All the coverage warnings make it difficult to review.

Comment on lines +92 to +99
output = ""
with io.StringIO() as buf:
buf.write(cls._class_stub())
for key in cls.__members__.keys():
buf.write(f" {key} = ...\n")
output = buf.getvalue()

return output
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
output = ""
with io.StringIO() as buf:
buf.write(cls._class_stub())
for key in cls.__members__.keys():
buf.write(f" {key} = ...\n")
output = buf.getvalue()
return output
result = [cls._class_stub()]
for key in cls.__members__.keys():
result.append(f" {key} = ...")
return "\n".join(result)

Should also get rid of the newline in _class_stub. Might be confusing for custom type implementations that you're responsible for ending with a newline.

@@ -31,6 +31,9 @@ homepage = "https://dissect.tools"
documentation = "https://docs.dissect.tools/en/latest/projects/dissect.cstruct"
repository = "https://github.com/fox-it/dissect.cstruct"

[project.scripts]
stubify = "dissect.cstruct.tools.stubify:main"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stubify = "dissect.cstruct.tools.stubify:main"
cstruct-stubify = "dissect.cstruct.tools.stubify:main"

Comment on lines +110 to +116
class __anonymous_0__(Union):
a: WcharArray
b: CharArray
def __init__(self, a: WcharArray=..., b: CharArray=...): ...
a: WcharArray
b: CharArray
def __init__(self, __anonymous_0__: __anonymous_0__=...): ...
Copy link
Member

Choose a reason for hiding this comment

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

Anonymous structures should get folded in, I don't think there's any use in them being typed?

@@ -181,6 +181,15 @@ def _write_0(cls, stream: BinaryIO, array: list[BaseType]) -> int:
"""
return cls._write_array(stream, array + [cls()])

def _class_stub(cls) -> str:
return f"class {cls.__name__}({cls.__base__.__name__}):\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return f"class {cls.__name__}({cls.__base__.__name__}):\n"
return f"class {cls.__name__}({cls.__base__.__name__}):"

@@ -84,6 +85,19 @@ def _write_0(cls, stream: BinaryIO, array: list[BaseType]) -> int:
data = [entry.value if isinstance(entry, Enum) else entry for entry in array]
return cls._write_array(stream, data + [cls.type()])

def _class_stub(cls) -> str:
return f"class {cls.__name__}({cls.__base__.__name__}, {cls.type.__name__}):\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return f"class {cls.__name__}({cls.__base__.__name__}, {cls.type.__name__}):\n"
return f"class {cls.__name__}({cls.__base__.__name__}, {cls.type.__name__}):"

Comment on lines +370 to +388
buffer = io.StringIO()
buffer.write(f"class {cls.__name__}({cls.__base__.__name__}):\n")
call_args = ["self"]
for key, field in cls.lookup.items():
if isinstance(field.type, StructureMetaType):
class_info = field.type.to_type_stub()
buffer.write(indent(class_info, prefix=" " * 4))
call_args.append(f"{field.type_stub()}=...")

for field in cls.fields.values():
type_info = field.type_stub()
buffer.write(indent(f"{type_info}\n", prefix=" " * 4))

call = ", ".join(call_args)
buffer.write(indent(f"def __init__({call}): ...\n", prefix=" " * 4))

output = buffer.getvalue()
buffer.close()
return output
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buffer = io.StringIO()
buffer.write(f"class {cls.__name__}({cls.__base__.__name__}):\n")
call_args = ["self"]
for key, field in cls.lookup.items():
if isinstance(field.type, StructureMetaType):
class_info = field.type.to_type_stub()
buffer.write(indent(class_info, prefix=" " * 4))
call_args.append(f"{field.type_stub()}=...")
for field in cls.fields.values():
type_info = field.type_stub()
buffer.write(indent(f"{type_info}\n", prefix=" " * 4))
call = ", ".join(call_args)
buffer.write(indent(f"def __init__({call}): ...\n", prefix=" " * 4))
output = buffer.getvalue()
buffer.close()
return output
result = [f"class {cls.__name__}({cls.__base__.__name__}):"]
args = ["self"]
for field in cls.fields.values():
if isinstance(field.type, StructureMetaType) and not field.type.__anonymous__:
result.append(indent(field.type.to_type_stub(), prefix=" " * 4))
result.append(f" {field._type_stub()}")
args.append(f"{field._type_stub()} = ...")
result.append(indent(f"def __init__({', '.join(args)}): ...", prefix=" " * 4))
return "\n".join(result)

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.

2 participants