Skip to content

Preserve tuple match narrows across subsequent cases #3224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Arths17 wants to merge 6 commits into
base: main
Choose a base branch
from
+84 −23
Open

Preserve tuple match narrows across subsequent cases #3224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7e6cc2
lsp: scope fix-all actions to pyrefly kind
Arths17 Apr 7, 2026
4948b1e
lsp: limit source.fixAll matching to pyrefly subtree
Arths17 Apr 7, 2026
bd89c04
lsp: require exact pyrefly fix-all kind match
Arths17 Apr 11, 2026
594c9cc
Preserve tuple match narrows across cases
Arths17 Apr 24, 2026
a9e47f6
Update pyrefly/lib/test/pattern_match.rs
Arths17 Apr 24, 2026
44d671a
Update pyrefly/lib/lsp/non_wasm/server.rs
Arths17 Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions pyrefly/lib/binding/pattern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,17 @@ impl<'a> BindingsBuilder<'a> {
Some(idx) => idx,
Option::None => {
// More patterns than tuple elements, skip narrowing
narrow_ops.and_all(self.bind_pattern(
MatchSubject::None,
x,
key_for_subpattern,
));
for (name, (op, range)) in self
.bind_pattern(
MatchSubject::None,
x,
key_for_subpattern,
)
.0
{
let subject = NarrowingSubject::Name(name);
narrow_ops.and_for_subject(&subject, op, range);
}
Comment on lines +222 to +226
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

In tuple/sequence pattern handling, this change switches from and_all to per-entry and_for_subject, which avoids Placeholder insertion. That means a multi-element pattern like case None, None will produce independent narrows for both names, and when stmt_match negates them for fallthrough it becomes a is not None AND b is not None (conjunction), which is not the logical negation of (a is None) AND (b is None). This can make later cases like case _, None incorrectly treat b as Never and can hide type errors. Consider representing fallthrough constraints at the tuple-pattern level (or otherwise avoid per-name negation for multi-name patterns) so the complement is modeled correctly.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@Arths17 Arths17 Apr 24, 2026

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

continue;
}
}
Expand All @@ -238,11 +244,13 @@ impl<'a> BindingsBuilder<'a> {
}
_ => MatchSubject::None,
};
narrow_ops.and_all(self.bind_pattern(
subject_for_subpattern,
x,
key_for_subpattern,
));
for (name, (op, range)) in self
.bind_pattern(subject_for_subpattern, x, key_for_subpattern)
.0
{
let subject = NarrowingSubject::Name(name);
narrow_ops.and_for_subject(&subject, op, range);
}
Comment on lines +249 to +253
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Same issue as above: collecting subpattern narrows without Placeholder insertion makes subsequent fallthrough narrowing treat per-element negations as conjunctive constraints. For tuple subjects this is not a sound representation of case exclusion and can lead to contradictory narrows (e.g., b != None carried into a case _, None).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@Arths17 Arths17 Apr 24, 2026

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

}
}
}
Expand Down Expand Up @@ -599,11 +607,13 @@ impl<'a> BindingsBuilder<'a> {
// shadow outer variables. When there is no narrowing subject
// (e.g. `match make_color():`), drop all narrows so that alias
// names don't resolve against unrelated outer variables.
new_narrow_ops.0.retain(|name, _| {
match_subject
.as_single()
.as_ref()
.is_some_and(|s| name == s.name())
new_narrow_ops.0.retain(|name, _| match &match_subject {
MatchSubject::Single(subject) => name == subject.name(),
MatchSubject::Tuple(subjects) => subjects
.iter()
.flatten()
.any(|subject| name == subject.name()),
Comment on lines +611 to +615
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Retaining per-element narrows for MatchSubject::Tuple means negated_prev_ops will accumulate per-name negations across cases, but those negations are applied conjunctively (bind_narrow_ops loops over names). For a prior case that narrowed multiple tuple elements (e.g. None, None), the correct fallthrough condition is a disjunction across elements, not a != None && b != None. This can make later cases unreachable or produce Never types. A different representation of tuple-case fallthrough (not per-name conjunction) is needed for sound narrowing across cases.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@Arths17 Arths17 Apr 24, 2026

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

MatchSubject::None => false,
});
negated_prev_ops.and_all(new_narrow_ops.negate());
self.stmts(body, parent);
Expand Down
44 changes: 37 additions & 7 deletions pyrefly/lib/lsp/non_wasm/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,11 @@ fn format_diagnostic_message_for_markdown(message: &str) -> String {

#[cfg(test)]
mod tests {
use lsp_types::CodeActionKind;

use super::SOURCE_FIX_ALL_PYREFLY;
use super::format_diagnostic_message_for_markdown;
use super::is_fix_all_code_action_kind_requested;

#[test]
fn test_format_diagnostic_message_for_markdown() {
Expand Down Expand Up @@ -722,6 +726,26 @@ mod tests {
fn test_format_only_special_characters() {
assert_eq!(format_diagnostic_message_for_markdown("***"), "\\*\\*\\*");
}

#[test]
fn test_fix_all_kind_filter_matches_supported_kinds() {
assert!(is_fix_all_code_action_kind_requested(
&CodeActionKind::SOURCE_FIX_ALL
));
assert!(is_fix_all_code_action_kind_requested(&CodeActionKind::new(
SOURCE_FIX_ALL_PYREFLY,
)));
}

#[test]
fn test_fix_all_kind_filter_rejects_pyrefly_suffix_kinds() {
assert!(!is_fix_all_code_action_kind_requested(
&CodeActionKind::new("source.fixAll.pyrefly.foo",)
));
assert!(!is_fix_all_code_action_kind_requested(
&CodeActionKind::new("source.fixAll.pyreflyyyyyy",)
));
}
}

pub struct Server {
Expand Down Expand Up @@ -1142,7 +1166,7 @@ pub fn capabilities(
CodeActionKind::new("refactor.delete"),
CodeActionKind::new("refactor.move"),
CodeActionKind::REFACTOR_INLINE,
CodeActionKind::SOURCE_FIX_ALL,
CodeActionKind::new(SOURCE_FIX_ALL_PYREFLY),
]),
..Default::default()
})),
Expand Down Expand Up @@ -1252,6 +1276,15 @@ pub enum ProcessEvent {
}

const PYTHON_SECTION: &str = "python";
const SOURCE_FIX_ALL_PYREFLY: &str = "source.fixAll.pyrefly";

fn is_fix_all_code_action_kind_requested(kind: &CodeActionKind) -> bool {
let requested = kind.as_str();
requested == SOURCE_FIX_ALL_PYREFLY
|| SOURCE_FIX_ALL_PYREFLY
.strip_prefix(requested)
.is_some_and(|suffix| suffix.starts_with('.'))
}

struct TypeHierarchyTarget {
def_index: ClassDefIndex,
Expand Down Expand Up @@ -4170,11 +4203,8 @@ impl Server {
let only_kinds = params.context.only.as_ref();
let allow_quickfix = only_kinds
.is_none_or(|kinds| kinds.iter().any(|kind| kind == &CodeActionKind::QUICKFIX));
let allow_fix_all = only_kinds.is_none_or(|kinds| {
kinds
.iter()
.any(|kind| kind == &CodeActionKind::SOURCE_FIX_ALL)
});
let allow_fix_all =
only_kinds.is_none_or(|kinds| kinds.iter().any(is_fix_all_code_action_kind_requested));
let allow_refactor = only_kinds.is_none_or(|kinds| {
kinds
.iter()
Expand Down Expand Up @@ -4276,7 +4306,7 @@ impl Server {
if !changes.is_empty() {
actions.push(CodeActionOrCommand::CodeAction(CodeAction {
title: "Remove all redundant casts".to_owned(),
kind: Some(CodeActionKind::SOURCE_FIX_ALL),
kind: Some(CodeActionKind::new(SOURCE_FIX_ALL_PYREFLY)),
edit: Some(WorkspaceEdit {
changes: Some(changes),
..Default::default()
Expand Down
2 changes: 1 addition & 1 deletion pyrefly/lib/test/lsp/lsp_interaction/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ fn test_initialize_basic() {
"definitionProvider": true,
"typeDefinitionProvider": true,
"codeActionProvider": {
"codeActionKinds": ["quickfix", "refactor.extract", "refactor.rewrite", "refactor.delete", "refactor.move", "refactor.inline", "source.fixAll"]
"codeActionKinds": ["quickfix", "refactor.extract", "refactor.rewrite", "refactor.delete", "refactor.move", "refactor.inline", "source.fixAll.pyrefly"]
},
"codeLensProvider": {
"resolveProvider": false,
Expand Down
21 changes: 21 additions & 0 deletions pyrefly/lib/test/pattern_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,27 @@ def test_sequence_after_none(seq_or_none: list[int] | None):
"#,
);

testcase!(
test_match_tuple_subject_narrowing_after_none,
r#"
from typing import assert_type

def test(a: list[int] | None, b: list[int] | None) -> None:
match a, b:
case None, None:
pass
case _, None:
assert_type(a, list[int])
assert_type(b, None)
case None, _:
Comment thread
Arths17 marked this conversation as resolved.
assert_type(a, None)
assert_type(b, list[int])
case _:
assert_type(a, list[int])
assert_type(b, list[int])
"#,
);

testcase!(
test_match_mapping_before_none,
r#"
Expand Down
Loading