Skip to content

Fix @no_type_check panic#3211

Closed
kinto0 wants to merge 1 commit intofacebook:mainfrom
kinto0:export-D102023871
Closed

Fix @no_type_check panic#3211
kinto0 wants to merge 1 commit intofacebook:mainfrom
kinto0:export-D102023871

Conversation

@kinto0
Copy link
Copy Markdown
Contributor

@kinto0 kinto0 commented Apr 23, 2026

Summary:
Add a regression test that reproduces the panic when running pyrefly report on functions decorated with no_type_check. The test currently asserts the panic behavior using #[should_panic].

When running pyrefly report on a function like:

from typing import no_type_check
no_type_check
def f(x: int):
    pass

The report command panics at pyrefly/lib/binding/bindings.rs:429 in key_to_idx_hashed with the error: "Internal error: key not found, module <module>, path <path>, key Annotation(ShortIdentifier(...))"

Root cause: The binding pass skips creating KeyAnnotation entries for parameters of no_type_check decorated functions (since their bodies are not analyzed), but parse_functions in report.rs unconditionally tries to look them up.

A follow-up diff will fix the panic and update this test to assert correct behavior instead of the panic.

Differential Revision: D102023871

Summary:
Add a regression test that reproduces the panic when running `pyrefly report` on functions decorated with `no_type_check`. The test currently asserts the panic behavior using `#[should_panic]`.

When running `pyrefly report` on a function like:
```python
from typing import no_type_check
no_type_check
def f(x: int):
    pass
```

The report command panics at `pyrefly/lib/binding/bindings.rs:429` in `key_to_idx_hashed` with the error: "Internal error: key not found, module `<module>`, path `<path>`, key Annotation(ShortIdentifier(...))"

Root cause: The binding pass skips creating `KeyAnnotation` entries for parameters of `no_type_check` decorated functions (since their bodies are not analyzed), but `parse_functions` in `report.rs` unconditionally tries to look them up.

A follow-up diff will fix the panic and update this test to assert correct behavior instead of the panic.

Differential Revision: D102023871
@meta-cla meta-cla Bot added the cla signed label Apr 23, 2026
@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented Apr 23, 2026

@kinto0 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D102023871.

@kinto0 kinto0 changed the title Add regression test for @no_type_check panic Fix @no_type_check panic Apr 23, 2026
@kinto0 kinto0 marked this pull request as draft April 23, 2026 15:51
@jorenham
Copy link
Copy Markdown
Contributor

Will symbols in @no_type_check functions/classes now be included in the report, or will they be ignored?

@kinto0
Copy link
Copy Markdown
Contributor Author

kinto0 commented Apr 23, 2026

Will symbols in @no_type_check functions/classes now be included in the report, or will they be ignored?

good question. I imagine it shouldn't. what do you think? cc @maggiemoss

@jorenham
Copy link
Copy Markdown
Contributor

Will symbols in @no_type_check functions/classes now be included in the report, or will they be ignored?

good question. I imagine it shouldn't. what do you think? cc @maggiemoss

Yea same, I think that those symbols should be ignored then.

@kinto0
Copy link
Copy Markdown
Contributor Author

kinto0 commented Apr 23, 2026

Will symbols in @no_type_check functions/classes now be included in the report, or will they be ignored?

good question. I imagine it shouldn't. what do you think? cc @maggiemoss

Yea same, I think that those symbols should be ignored then.

did you still want to take this over? or do you want me to finish it

@jorenham
Copy link
Copy Markdown
Contributor

jorenham commented Apr 23, 2026

Will symbols in @no_type_check functions/classes now be included in the report, or will they be ignored?

good question. I imagine it shouldn't. what do you think? cc @maggiemoss

Yea same, I think that those symbols should be ignored then.

did you still want to take this over? or do you want me to finish it

I guess that depends on how quickly you are able to get this in. Does next week sound realistic?

@kinto0 kinto0 self-assigned this Apr 23, 2026
@kinto0 kinto0 closed this Apr 27, 2026
@kinto0
Copy link
Copy Markdown
Contributor Author

kinto0 commented Apr 27, 2026

closed from other fix, referenced in #3187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants