Prevent panic on malformed empty parameter names during binding solve#2998
Prevent panic on malformed empty parameter names during binding solve#2998Arths17 wants to merge 4 commits intofacebook:mainfrom
Conversation
…d a regression test for the fuzzed snippet, confirmed the repro no longer panics (it now emits parse/type errors), and completed formatting/lint checks with jsonschema skipped due missing local Python deps.
modified: pyrefly/lib/binding/bindings.rs modified: pyrefly/lib/test/simple.rs deleted: test_crash.py
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in Pyrefly’s binding/solve pipeline caused by parser-recovery input that can synthesize an empty parameter name, leaving a promised Key::Definition without an associated binding (leading to a panic during solve).
Changes:
- Add a pre-solve repair step that fills only malformed empty-range
Key::Definitionpromises withAny(Error)while preserving invariant panics for other missing bindings. - Skip creating parameter bindings when a parameter name is synthesized/empty to avoid creating dangling entries.
- Add a regression test covering the fuzzed repro from issue #2984.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/binding/bindings.rs |
Adds a repair pass for missing bindings and avoids binding empty/synthesized parameter names. |
pyrefly/lib/test/simple.rs |
Adds a regression test ensuring the fuzzed snippet no longer panics. |
.vscode/tasks.json |
Simplifies the Rust problem matcher configuration for the cargo watch task. |
.gitignore |
Ignores venv/ (but should also ignore Python bytecode/cache artifacts given this PR). |
__pycache__/test.cpython-312-pytest-9.0.2.pyc |
Adds a compiled Python bytecode artifact (should not be committed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,4 @@ | |||
| debug.json | |||
| debug.js | |||
| venv/ | |||
| } | ||
| } | ||
| }, | ||
| "problemMatcher": { |
| for (idx, key) in missing { | ||
| match key { | ||
| Key::Definition(short_identifier) if short_identifier.range().is_empty() => { | ||
| self.insert_binding_idx(idx, Binding::Any(AnyStyle::Error)); |
There was a problem hiding this comment.
i believe we panic if we do already have a binding inserted, so i think we need to check that.
|
thanks for the PR, I left some comments but I think no action needed from you ATM since someone else already made a change that fixes this, and we need to decide which fix to use |
|
@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D99469042. |
|
I have some reservations with this approach, since it's kind of a cleanup that hides mistakes in other parts of the system. I think i would prefer something that prevents definitions w/ empty keys from being created at all |
|
Thanks for the review, @yangdanny97! That makes total sense preventing the empty keys from being created in the first place is definitely a cleaner long-term fix. I saw the note about another potential fix being in the works. I'm happy to hold off for now while you guys decide on the best direction, or if you'd like, I can take a stab at moving the logic earlier into the pipeline to prevent the creation of those definitions. Just let me know how you'd like to proceed! |
Summary:
Implemented and validated the crash fix for issue #2984, added a regression test for the fuzzed snippet, confirmed the repro no longer panics (it now emits parse/type errors), and completed formatting/lint checks with jsonschema skipped due missing local Python deps.
Fix a crash in the binding/solve pipeline for malformed parser-recovery input where an empty synthesized parameter name could leave a promised definition key without a corresponding binding.
This PR adds a repair step before solve that:
Fills only malformed empty-name definition promises with an error binding.
Preserves strict invariant checking by still panicking on any other missing binding.
Also adds a regression test using the fuzzed repro from the issue to ensure this no longer panics and instead reports normal parse/type diagnostics.
Fixes #2984
Test Plan
Ran targeted regression test:
cargo test -p pyrefly test_crash_on_fuzzed_empty_parameter_name -- --nocapture
Re-ran the original fuzzed snippet via CLI:
cargo run -q -p pyrefly -- snippet $'def f(\n @\na:b:@or1:c=1\n'
Verified: no panic; parse/type errors are reported.
Ran formatting and lint checks:
python3 test.py --no-test --no-conformance --no-jsonschema
Note:
jsonschema checks were skipped locally because required Python packages were not installed in this environment. is this good for a pr