ronantakizawa/codereview-bench huggingface dataset:
Repository: mandiant/capa
File: capa/features/address.py (Python)

Code before:
        return "process(%s%s)" % (
            f"ppid: {self.ppid}, " if self.ppid > 0 else "",
            f"pid: {self.pid}",
        )

    def __hash__(self):
        return hash((self.ppid, self.pid))

    def __eq__(self, other):
        assert isinstance(other, ProcessAddress)
        if self.ppid > 0:
            return (self.ppid, self.pid) == (other.ppid, other.pid)
        else:
            return self.pid == other.pid

    def __lt__(self, other):
        return (self.ppid, self.pid) < (other.ppid, other.pid)


class ThreadAddress(Address):
    """addresses a thread in a dynamic execution trace"""

    def __init__(self, process: ProcessAddress, tid: int):
        assert tid >= 0
        self.ppid = process.ppid
        self.pid = process.pid
        self.tid = tid

    def __repr__(self):
        return f"thread(tid: {self.tid})"

    def __hash__(self):
        return hash((self.ppid, self.pid, self.tid))

    def __eq__(self, other):
        assert isinstance(other, ThreadAddress)
        return (self.ppid, self.pid, self.tid) == (other.ppid, other.pid, other.tid)

    def __lt__(self, other):
        return (self.ppid, self.pid, self.tid) < (other.ppid, other.pid, other.tid)


class DynamicAddress(Address):
    """an address from a dynamic analysis trace"""

    def __init__(self, id_: int, return_address: int):
        assert id_ >= 0
        assert return_address >= 0
        self.id = id_
        self.return_address = return_address


Review comment (suggestion):
```suggestion
        self.process = process
```

Code after:
        return "process(%s%s)" % (
            f"ppid: {self.ppid}, " if self.ppid > 0 else "",
            f"pid: {self.pid}",
        )

    def __hash__(self):
        return hash((self.ppid, self.pid))

    def __eq__(self, other):
        assert isinstance(other, ProcessAddress)
        return (self.ppid, self.pid) == (other.ppid, other.pid)

    def __lt__(self, other):
        return (self.ppid, self.pid) < (other.ppid, other.pid)


class ThreadAddress(Address):
    """addresses a thread in a dynamic execution trace"""

    def __init__(self, process: ProcessAddress, tid: int):
        assert tid >= 0
        self.process = process
        self.tid = tid

    def __repr__(self):
        return f"thread(tid: {self.tid})"

    def __hash__(self):
        return hash((self.process, self.tid))

    def __eq__(self, other):
        assert isinstance(other, ThreadAddress)
        return (self.process, self.tid) == (other.process, other.tid)

    def __lt__(self, other):
        return (self.process, self.tid) < (other.process, other.tid)


class DynamicAddress(Address):
    """an address from a dynamic analysis trace"""

    def __init__(self, id_: int, return_address: int):
        assert id_ >= 0
        assert return_address >= 0
        self.id = id_
        self.return_address = return_address

    def __repr__(self):
        return f"dynamic(event: {self.id}, returnaddress: 0x{self.return_address:x})"

    def __hash__(self):
Repository: mandiant/capa
File: capa/features/address.py (Python)

Code before:
    def __hash__(self):
        # implement hash so that addresses can be used in sets and dicts
        ...

    @abc.abstractmethod
    def __repr__(self):
        # implement repr to help during debugging
        ...


class AbsoluteVirtualAddress(int, Address):
    """an absolute memory address"""

    def __new__(cls, v):
        assert v >= 0
        return int.__new__(cls, v)

    def __repr__(self):
        return f"absolute(0x{self:x})"

    def __hash__(self):
        return int.__hash__(self)


class ProcessAddress(Address):
    """addresses a processes in a dynamic execution trace"""

    def __init__(self, pid: int, ppid: int = 0):
        assert ppid >= 0
        assert pid > 0
        self.ppid = ppid
        self.pid = pid

    def __repr__(self):
        return "process(%s%s)" % (
            f"ppid: {self.ppid}, " if self.ppid > 0 else "",
            f"pid: {self.pid}",
        )

    def __hash__(self):
        return hash((self.ppid, self.pid))

    def __eq__(self, other):
        assert isinstance(other, ProcessAddress)
        return (self.ppid, self.pid) == (other.ppid, other.pid)

    def __lt__(self, other):
        return (self.ppid, self.pid) < (other.ppid, other.pid)


class ThreadAddress(Address):

Review comment (suggestion):
```suggestion
    """an address of a process in a dynamic execution trace"""
```

Code after:
    def __hash__(self):
        # implement hash so that addresses can be used in sets and dicts
        ...

    @abc.abstractmethod
    def __repr__(self):
        # implement repr to help during debugging
        ...


class AbsoluteVirtualAddress(int, Address):
    """an absolute memory address"""

    def __new__(cls, v):
        assert v >= 0
        return int.__new__(cls, v)

    def __repr__(self):
        return f"absolute(0x{self:x})"

    def __hash__(self):
        return int.__hash__(self)


class ProcessAddress(Address):
    """an address of a process in a dynamic execution trace"""

    def __init__(self, pid: int, ppid: int = 0):
        assert ppid >= 0
        assert pid > 0
        self.ppid = ppid
        self.pid = pid

    def __repr__(self):
        return "process(%s%s)" % (
            f"ppid: {self.ppid}, " if self.ppid > 0 else "",
            f"pid: {self.pid}",
        )

    def __hash__(self):
        return hash((self.ppid, self.pid))

    def __eq__(self, other):
        assert isinstance(other, ProcessAddress)
        return (self.ppid, self.pid) == (other.ppid, other.pid)

    def __lt__(self, other):
        return (self.ppid, self.pid) < (other.ppid, other.pid)


class ThreadAddress(Address):
Repository: mandiant/capa
File: capa/render/result_document.py (Python)

Code before:
class Metadata(Model):
    timestamp: datetime.datetime
    version: str
    argv: Optional[Tuple[str, ...]] = None
    sample: Sample
    analysis: Analysis


class CompoundStatementType:
    AND = "and"
    OR = "or"
    NOT = "not"
    OPTIONAL = "optional"


class StatementModel(FrozenModel):
    ...


class CompoundStatement(StatementModel):
    type: str
    description: Optional[str] = None


class SomeStatement(StatementModel):
    type: str = "some"
    description: Optional[str] = None
    count: int


class RangeStatement(StatementModel):
    type: str = "range"
    description: Optional[str] = None
    min: int
    max: int
    child: frz.Feature


class SubscopeStatement(StatementModel):
    type: str = "subscope"
    description: Optional[str] = None
    scope: capa.rules.Scope


Statement = Union[
    # Note! order matters, see #1161
    RangeStatement,
    SomeStatement,
    SubscopeStatement,
    CompoundStatement,
]

Review comment (suggestion):
i dont think this is equivalent. is it important that this change be made?

the type before the change would be `typing.Literal["some"]` and after the change `str`. 

Code after:
class Metadata(Model):
    timestamp: datetime.datetime
    version: str
    argv: Optional[Tuple[str, ...]]
    sample: Sample
    analysis: Analysis


class CompoundStatementType:
    AND = "and"
    OR = "or"
    NOT = "not"
    OPTIONAL = "optional"


class StatementModel(FrozenModel):
    ...


class CompoundStatement(StatementModel):
    type: str
    description: Optional[str] = None


class SomeStatement(StatementModel):
    type: Literal["some"] = "some"
    description: Optional[str] = None
    count: int


class RangeStatement(StatementModel):
    type: Literal["range"] = "range"
    description: Optional[str] = None
    min: int
    max: int
    child: frz.Feature


class SubscopeStatement(StatementModel):
    type: Literal["subscope"] = "subscope"
    description: Optional[str] = None
    scope: capa.rules.Scope


Statement = Union[
    # Note! order matters, see #1161
    RangeStatement,
    SomeStatement,
    SubscopeStatement,
    CompoundStatement,
]
Repository: mandiant/capa
File: capa/render/result_document.py (Python)

Code before:
class FunctionFeatureCount(Model):
    address: frz.Address
    count: int


class FeatureCounts(Model):
    file: int
    functions: Tuple[FunctionFeatureCount, ...]


class Analysis(Model):
    format: str
    arch: str
    os: str
    extractor: str
    rules: Tuple[str, ...]
    base_address: frz.Address
    layout: Layout
    feature_counts: FeatureCounts
    library_functions: Tuple[LibraryFunction, ...]


class Metadata(Model):
    timestamp: datetime.datetime
    version: str
    argv: Optional[Tuple[str, ...]] = None
    sample: Sample
    analysis: Analysis


class CompoundStatementType:
    AND = "and"
    OR = "or"
    NOT = "not"
    OPTIONAL = "optional"


class StatementModel(FrozenModel):
    ...


class CompoundStatement(StatementModel):
    type: str
    description: Optional[str] = None


class SomeStatement(StatementModel):
    type: str = "some"
    description: Optional[str] = None
    count: int


Review comment (bug):
can you explain this change? the constructors of this data should be setting the property `argv`. we could relax that, but unless there's already a bug in our code, i dont think this is necessary?

Code after:
class FunctionFeatureCount(Model):
    address: frz.Address
    count: int


class FeatureCounts(Model):
    file: int
    functions: Tuple[FunctionFeatureCount, ...]


class Analysis(Model):
    format: str
    arch: str
    os: str
    extractor: str
    rules: Tuple[str, ...]
    base_address: frz.Address
    layout: Layout
    feature_counts: FeatureCounts
    library_functions: Tuple[LibraryFunction, ...]


class Metadata(Model):
    timestamp: datetime.datetime
    version: str
    argv: Optional[Tuple[str, ...]]
    sample: Sample
    analysis: Analysis


class CompoundStatementType:
    AND = "and"
    OR = "or"
    NOT = "not"
    OPTIONAL = "optional"


class StatementModel(FrozenModel):
    ...


class CompoundStatement(StatementModel):
    type: str
    description: Optional[str] = None


class SomeStatement(StatementModel):
    type: Literal["some"] = "some"
    description: Optional[str] = None
    count: int
Repository: mandiant/capa
File: scripts/show-unused-features.py (Python)

Code before:
    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    highlight_unused_features(feature_map, rules_feature_set)

    return 0


def highlight_unused_features(feature_map: dict, rules_feature_set: set):
    unused_features = []
    for feature, count in feature_map.items():
        if feature in rules_feature_set:
            continue
        unused_features.append((get_colored(str(count)), get_colored(feature.__str__())))
    print("\n")
    print(tabulate.tabulate(unused_features, headers=["Count", "Feature"], tablefmt="plain"))
    print("\n")


def get_colored(s: str):
    if "(" in s:
        s_split = s.split("(")
        s_split[0] = colored(s_split[0], "cyan")
        return "(".join(s_split)
    else:
        return colored(s, "cyan")


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())


Review comment (suggestion):
would you move these before the `main` implementations?

we typically order the code so that dependencies are first, and the higher level routines at the end. so with this ordering, `get_colored` goes first and then `highlight_unused_features`.

Code after:
    feature_map: typing.Counter[Feature] = Counter()

    feature_map.update([feature for feature, _ in extractor.extract_file_features()])

    function_handles = tuple(extractor.get_functions())

    if function:
        function_handles = tuple(filter(lambda fh: fh.inner.start_ea == function, function_handles))

        if len(function_handles) == 0:
            print(f"{hex(function)} not a function")
            return -1

    feature_map.update(get_file_features(function_handles, extractor))

    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    print_unused_features(feature_map, rules_feature_set)

    return 0


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())
Repository: mandiant/capa
File: scripts/show-unused-features.py (Python)

Code before:

    if not function:
        for feature, _ in extractor.extract_file_features():
            feature_map[feature] = feature_map.get(feature, 0) + 1
        return

    function_handles = tuple(extractor.get_functions())

    if function:
        function_handles = tuple(filter(lambda fh: fh.inner.start_ea == function, function_handles))

        if len(function_handles) == 0:
            print(f"{hex(function)} not a function")
            return -1

    feature_map.update(get_file_features(function_handles, extractor))

    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    highlight_unused_features(feature_map, rules_feature_set)

    return 0


def highlight_unused_features(feature_map: dict, rules_feature_set: set):
    unused_features = []
    for feature, count in feature_map.items():
        if feature in rules_feature_set:
            continue
        unused_features.append((get_colored(str(count)), get_colored(feature.__str__())))
    print("\n")
    print(tabulate.tabulate(unused_features, headers=["Count", "Feature"], tablefmt="plain"))
    print("\n")


def get_colored(s: str):
    if "(" in s:
        s_split = s.split("(")
        s_split[0] = colored(s_split[0], "cyan")
        return "(".join(s_split)
    else:
        return colored(s, "cyan")


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())


Review comment (suggestion):
```suggestion
def highlight_unused_features(feature_map: Dict, rules_feature_set: Set):
```

using the collection name, like `set` and `dict`, is only supported from Python 3.9+, so we need to keep using things like `Set` and `Dict` until 3.8 is deprecated.

Code after:

    rules_feature_set = get_rules_feature_set(args.rules)

    print_unused_features(feature_map, rules_feature_set)
    return 0


def ida_main():
    import idc

    import capa.main
    import capa.features.extractors.ida.extractor

    function = idc.get_func_attr(idc.here(), idc.FUNCATTR_START)
    print(f"getting features for current function {hex(function)}")

    extractor = capa.features.extractors.ida.extractor.IdaFeatureExtractor()
    feature_map: typing.Counter[Feature] = Counter()

    feature_map.update([feature for feature, _ in extractor.extract_file_features()])

    function_handles = tuple(extractor.get_functions())

    if function:
        function_handles = tuple(filter(lambda fh: fh.inner.start_ea == function, function_handles))

        if len(function_handles) == 0:
            print(f"{hex(function)} not a function")
            return -1

    feature_map.update(get_file_features(function_handles, extractor))

    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    print_unused_features(feature_map, rules_feature_set)

    return 0


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())
Repository: mandiant/capa
File: scripts/show-unused-features.py (Python)

Code before:

    function_handles = tuple(extractor.get_functions())

    if function:
        function_handles = tuple(filter(lambda fh: fh.inner.start_ea == function, function_handles))

        if len(function_handles) == 0:
            print(f"{hex(function)} not a function")
            return -1

    feature_map.update(get_file_features(function_handles, extractor))

    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    highlight_unused_features(feature_map, rules_feature_set)

    return 0


def highlight_unused_features(feature_map: dict, rules_feature_set: set):
    unused_features = []
    for feature, count in feature_map.items():
        if feature in rules_feature_set:
            continue
        unused_features.append((get_colored(str(count)), get_colored(feature.__str__())))
    print("\n")
    print(tabulate.tabulate(unused_features, headers=["Count", "Feature"], tablefmt="plain"))
    print("\n")


def get_colored(s: str):
    if "(" in s:
        s_split = s.split("(")
        s_split[0] = colored(s_split[0], "cyan")
        return "(".join(s_split)
    else:
        return colored(s, "cyan")


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())


Review comment (suggestion):
```suggestion
        unused_features.append((get_colored(str(count)), get_colored(str(feature))))
```

Code after:


def ida_main():
    import idc

    import capa.main
    import capa.features.extractors.ida.extractor

    function = idc.get_func_attr(idc.here(), idc.FUNCATTR_START)
    print(f"getting features for current function {hex(function)}")

    extractor = capa.features.extractors.ida.extractor.IdaFeatureExtractor()
    feature_map: typing.Counter[Feature] = Counter()

    feature_map.update([feature for feature, _ in extractor.extract_file_features()])

    function_handles = tuple(extractor.get_functions())

    if function:
        function_handles = tuple(filter(lambda fh: fh.inner.start_ea == function, function_handles))

        if len(function_handles) == 0:
            print(f"{hex(function)} not a function")
            return -1

    feature_map.update(get_file_features(function_handles, extractor))

    rules_path = capa.main.get_default_root() / "rules"
    rules_feature_set = get_rules_feature_set([rules_path])

    print_unused_features(feature_map, rules_feature_set)

    return 0


if __name__ == "__main__":
    if capa.helpers.is_runtime_ida():
        ida_main()
    else:
        sys.exit(main())
Repository: mandiant/capa
File: capa/features/common.py (Python)

Code before:
    reordered_hex_pairs = [
        h[3],
        h[2],
        h[1],
        h[0],
        h[5],
        h[4],
        h[7],
        h[6],
        h[8],
        h[9],
        h[10],
        h[11],
        h[12],
        h[13],
        h[14],
        h[15],
    ]
    guid_bytes = bytes.fromhex("".join(reordered_hex_pairs))
    return guid_string, guid_bytes


class COMFactory:
    def __new__(cls, com_name: str, com_type: str):
        guid_string, guid_bytes = get_guid_string_and_bytes(com_name, com_type)
        return capa.engine.Or([Bytes(guid_bytes, com_name), String(guid_string, com_name)])


# other candidates here: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types
ARCH_I386 = "i386"
ARCH_AMD64 = "amd64"
# dotnet
ARCH_ANY = "any"
VALID_ARCH = (ARCH_I386, ARCH_AMD64, ARCH_ANY)


class Arch(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "arch"


OS_WINDOWS = "windows"
OS_LINUX = "linux"
OS_MACOS = "macos"
# dotnet
OS_ANY = "any"
VALID_OS = {os.value for os in capa.features.extractors.elf.OS}
VALID_OS.update({OS_WINDOWS, OS_LINUX, OS_MACOS, OS_ANY})
# internal only, not to be used in rules
OS_AUTO = "auto"

Review comment (suggestion):
please provide descriptions to the newly generated terms.

the `Or` should get the comment from the original term, and then `bytes` and `string` terms should get comments like `IID_IWebBrowser GUID as bytes`, etc.

Code after:
OS_ANY = "any"
VALID_OS = {os.value for os in capa.features.extractors.elf.OS}
VALID_OS.update({OS_WINDOWS, OS_LINUX, OS_MACOS, OS_ANY})
# internal only, not to be used in rules
OS_AUTO = "auto"


class OS(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "os"

    def evaluate(self, ctx, **kwargs):
        capa.perf.counters["evaluate.feature"] += 1
        capa.perf.counters["evaluate.feature." + self.name] += 1

        for feature, locations in ctx.items():
            if not isinstance(feature, (OS,)):
                continue

            assert isinstance(feature.value, str)
            if OS_ANY in (self.value, feature.value) or self.value == feature.value:
                return Result(True, self, [], locations=locations)

        return Result(False, self, [])


FORMAT_PE = "pe"
FORMAT_ELF = "elf"
FORMAT_DOTNET = "dotnet"
VALID_FORMAT = (FORMAT_PE, FORMAT_ELF, FORMAT_DOTNET)
# internal only, not to be used in rules
FORMAT_AUTO = "auto"
FORMAT_SC32 = "sc32"
FORMAT_SC64 = "sc64"
FORMAT_FREEZE = "freeze"
FORMAT_RESULT = "result"
FORMAT_UNKNOWN = "unknown"


class Format(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "format"


def is_global_feature(feature):
    """
    is this a feature that is extracted at every scope?
    today, these are OS and arch features.
    """
Repository: mandiant/capa
File: capa/features/common.py (Python)

Code before:
    reordered_hex_pairs = [
        h[3],
        h[2],
        h[1],
        h[0],
        h[5],
        h[4],
        h[7],
        h[6],
        h[8],
        h[9],
        h[10],
        h[11],
        h[12],
        h[13],
        h[14],
        h[15],
    ]
    guid_bytes = bytes.fromhex("".join(reordered_hex_pairs))
    return guid_string, guid_bytes


class COMFactory:
    def __new__(cls, com_name: str, com_type: str):
        guid_string, guid_bytes = get_guid_string_and_bytes(com_name, com_type)
        return capa.engine.Or([Bytes(guid_bytes, com_name), String(guid_string, com_name)])


# other candidates here: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types
ARCH_I386 = "i386"
ARCH_AMD64 = "amd64"
# dotnet
ARCH_ANY = "any"
VALID_ARCH = (ARCH_I386, ARCH_AMD64, ARCH_ANY)


class Arch(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "arch"


OS_WINDOWS = "windows"
OS_LINUX = "linux"
OS_MACOS = "macos"
# dotnet
OS_ANY = "any"
VALID_OS = {os.value for os in capa.features.extractors.elf.OS}
VALID_OS.update({OS_WINDOWS, OS_LINUX, OS_MACOS, OS_ANY})
# internal only, not to be used in rules
OS_AUTO = "auto"

Review comment (refactor):
all this is only used by the rules parser, so please move this into `capa/rules/__init__.py`. it should go in this common file if its used by more than one other file.

Code after:
OS_ANY = "any"
VALID_OS = {os.value for os in capa.features.extractors.elf.OS}
VALID_OS.update({OS_WINDOWS, OS_LINUX, OS_MACOS, OS_ANY})
# internal only, not to be used in rules
OS_AUTO = "auto"


class OS(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "os"

    def evaluate(self, ctx, **kwargs):
        capa.perf.counters["evaluate.feature"] += 1
        capa.perf.counters["evaluate.feature." + self.name] += 1

        for feature, locations in ctx.items():
            if not isinstance(feature, (OS,)):
                continue

            assert isinstance(feature.value, str)
            if OS_ANY in (self.value, feature.value) or self.value == feature.value:
                return Result(True, self, [], locations=locations)

        return Result(False, self, [])


FORMAT_PE = "pe"
FORMAT_ELF = "elf"
FORMAT_DOTNET = "dotnet"
VALID_FORMAT = (FORMAT_PE, FORMAT_ELF, FORMAT_DOTNET)
# internal only, not to be used in rules
FORMAT_AUTO = "auto"
FORMAT_SC32 = "sc32"
FORMAT_SC64 = "sc64"
FORMAT_FREEZE = "freeze"
FORMAT_RESULT = "result"
FORMAT_UNKNOWN = "unknown"


class Format(Feature):
    def __init__(self, value: str, description=None):
        super().__init__(value, description=description)
        self.name = "format"


def is_global_feature(feature):
    """
    is this a feature that is extracted at every scope?
    today, these are OS and arch features.
    """