Skip to content

crisp: fix many type errors found by mypy and ty - #11

Open
kkysen wants to merge 20 commits into
kkysen/ci-rufffrom
kkysen/ci-ty
Open

crisp: fix many type errors found by mypy and ty#11
kkysen wants to merge 20 commits into
kkysen/ci-rufffrom
kkysen/ci-ty

Conversation

@kkysen

@kkysen kkysen commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

This adds the mypy (and associated *-types packages needed to help it) and ty type checkers as dev dependencies and fixes almost all of the type errors found by both of them (under non-strict mode). It doesn't add them to CI yet, though, as there are still some errors that I'm not sure how to resolve. But fixing some of these first is helpful, especially 58717b8, as that uncovered a bug. These can be run locally with uv run mypy crisp and uv run ty check.

I think this should supersede most of #1, as the code has changed a lot since then, and what type checker was being used there wasn't totally clear.

@kkysen
kkysen requested a review from spernsteiner November 17, 2025 22:38

@spernsteiner spernsteiner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is very helpful—thank you!

Comment thread crisp/mvir.py
Comment on lines -625 to +631
c_code: NodeId
cmd: list[str]
exit_code: int
compile_commands: Optional[NodeId]

c_code = property(lambda self: self._metadata["c_code"])
cmd = property(lambda self: self._metadata["cmd"])
exit_code = property(lambda self: self._metadata["exit_code"])
compile_commands = property(lambda self: self._metadata["compile_commands"])
@property
def c_code(self) -> NodeId:
return self._metadata["c_code"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC there's logic somewhere that looks at the field type annotations on Node subclasses—will that work properly with the new style of field declarations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm not sure. Let me see. It might have to be adjusted, but the previous way didn't type-check (the field declaration is different from the field = property(), which is function).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I've fixed it in 8b9a3bb and 0205e11. Now _all_field_types should return the same thing as before. Is that the only place this is done, or are there others, too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried this approach and it seems to work:

def node_prop(name: str):
    return property(lambda self: self._metadata[name])

class FindUnsafeAnalysisNode(Node):
    KIND = "find_unsafe_analysis"

    code: NodeId = node_prop('code')
    commit: str = node_prop('commit')
    stderr: str = node_prop('stderr')
    # `body` stores the JSON output

Less verbose, and it keeps typing.get_type_hints working as before. mypy and ty check both accept it—they don't complain about the assignment in the class definition (e.g. code = node_prop('code')), and do complain about misuse of field values, like this:

def test_mypy(x: FindUnsafeAnalysisNode) -> int:
    return x.stderr
    # ^ error: Incompatible return value type (got "str", expected "int")

AIUI this works specifically because node_prop has no annotation on its return type, so mypy/ty won't check the code = node_prop('code') assignment. If you annotate node_prop with -> property then both tools complain about the assignment.

Comment thread .gitignore Outdated
@kkysen
kkysen force-pushed the kkysen/ci-ty branch 2 times, most recently from d82778a to 96e7594 Compare November 18, 2025 15:41
@spernsteiner

Copy link
Copy Markdown
Contributor

I tested this locally and ran into some errors about kind vs KIND in node metadata. @kkysen, can you look into this when you have some time? I think you should be able to reproduce it by running test_eval_20250917.py with an older version of CRISP and then running test_eval_20250917.py again with this branch, on the same project and without deleting crisp-storage.

I also hit an error about recursive imports in crisp.sandbox, which I worked around by moving the definition of the Sandbox class (which crisp.sandbox.docker needs) above the point where crisp.sandbox.docker gets imported into crisp.sandbox.

@kkysen kkysen left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay I can take a look. I still need to rebase it, though, as things got pretty out of date.

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