feat: suppress implicit-any-parameter for underscore-prefixed params#3574
feat: suppress implicit-any-parameter for underscore-prefixed params#3574MD-Mushfiqur123 wants to merge 3 commits into
Conversation
Parameters starting with a single underscore that do not end with underscore are conventionally used to indicate intentionally unused variables (e.g. , ). Skip the implicit-any-parameter error for these names to reduce noise, while still flagging dunders (), sunders (), and other underscore patterns. Fixes facebook#3542
| // as they indicate intentionally unused variables. | ||
| // `_`, `_abc` → skip; `__abc`, `_abc_`, `__abc__` → don't skip. | ||
| let is_underscore_unused = { | ||
| let b = name.as_bytes(); |
There was a problem hiding this comment.
name.starts_with("_") && (name.len() == 1 || !name.ends_with("_")) is easier IMO, none of the other dunder/sunder checking in Pyrefly converts to bytes so I don't think we should break that pattern
There was a problem hiding this comment.
also, can we refactor this to be a helper inside
https://github.com/facebook/pyrefly/blob/main/crates/pyrefly_python/src/ast.rs#L414
with a function name like maybe_intentionally_unused that way we can reuse the check in the LSP?
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
Back to you with one comment, once that's addressed we can merge
|
Oh one more thing. We should avoid emitting unused-variable and unused-parameter diagnostics for these names in the language server as well pyrefly/pyrefly/lib/lsp/non_wasm/server.rs Line 5250 in ef0fcfe pyrefly/pyrefly/lib/lsp/non_wasm/server.rs Line 5298 in ef0fcfe You can reuse the helper function for that |
…derscore-prefixed names
|
Friendly ping — this PR is ready for review. All CI checks are passing. |
|
The LSP suppression has been implemented in the latest commit (suppress unused-parameter and unused-variable diagnostics for underscore-prefixed names). Could you please re-review? |
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
You didn't address my comments in function.rs. The logic in server.rs is also inconsistent with function.rs - it should be the same, which is why I suggested you make a helper function.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@yangdanny97 friendly ping — changes have been pushed addressing the LSP suppression feedback. Could you take another look? |
|
@MD-Mushfiqur123 I took another look - you didn't address all the comments from my initial review |
Per review feedback, the intentionally-unused check is now a single function in ast.rs, used by both the type checker (function.rs) and the LSP (server.rs). The check itself follows the reviewer's simpler suggestion: .
|
@yangdanny97 All review comments addressed:
Could you take another look? |
3 participants
Summary
Parameters starting with a single underscore that do not end with underscore are conventionally used to indicate intentionally unused variables. This PR suppresses the
implicit-any-parametererror for such parameters, reducing noise for a common Python pattern.Fixes #3542
Changes
pyrefly/lib/alt/function.rs: Skip emittingImplicitAnyParameterfor parameters where the name starts with exactly one underscore and does not end with underscore (e.g._,_x)pyrefly/lib/test/inference.rs: Add test cases covering all underscore patternsUnderscore patterns
__abc__abc_abc___abc__xCLA
I will submit the CLA if needed.
🤖 This PR was created with AI assistance