Skip to content

Support indirect match subjects (match get_result())#3865

Open
lennardwalter wants to merge 1 commit into
facebook:mainfrom
primorio:indirect-match-targets
Open

Support indirect match subjects (match get_result())#3865
lennardwalter wants to merge 1 commit into
facebook:mainfrom
primorio:indirect-match-targets

Conversation

@lennardwalter

Copy link
Copy Markdown

Summary

Adds support for narrowing indirect match subjects, such as match get_result(): and match await get_result(): etc.

Previously, match narrowing depended on the subject being recoverable as a normal narrowing subject, like match x:. Non-name subjects did not have a stable internal subject identity, even though Python evaluates the match subject exactly once before matching. This meant Pyrefly could lose case-by-case narrowing information for exhaustiveness and control-flow analysis.

This PR introduces a synthetic internal match subject for non-name subjects. The synthetic subject lets the existing narrowing and exhaustiveness machinery treat the evaluated match expression like a stable temporary, while diagnostics still point at and display the original source expression.

This fixes cases where exhaustive matches over indirect subjects were incorrectly treated as possibly falling through, and enables non-exhaustive diagnostics for indirect subjects to refer to the real matched expression.

Fixes #3730

Test Plan

  • Added tests and ran full suite. Also modified existing tests as now non-exhaustiveness is correctly reported for some.

Follow-Up: Cleaner Synthetic Match Subject Representation

The implementation currently represents the internal temporary subject with a synthetic name, "$match_subject". This is intentionally local-only and not user-facing, but it is still somewhat ad hoc.
A cleaner design would represent synthetic match subjects as a first-class internal narrowing subject, probably backed by the already-created Idx<Key> for the evaluated match expression. For example, NarrowingSubject could gain a temporary/internal variant rather than encoding the temporary as a fake Name.
Doing that properly likely requires changing NarrowOps to key by a subject identity instead of only by Name, since the current narrowing flow frequently looks up entries via narrowing_subject.name(). A future cleanup could introduce a NarrowingSubjectKey that can represent both real names and internal temporaries, removing the need for the string sentinel entirely. IMO it is fine like it is.

@github-actions

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

narrowing_subject: narrowing_subject.clone(),
narrow_ops_for_fall_through: (Box::new(op.clone()), *range),
subject_range: x.subject.range(),
subject_range: match_subject.display_subject_range(x.subject.range()),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're effectively matching on match_subject twice here but one of them is hidden. IMO, adding something like

let (subject_range, display_subject_range) = match match_subject {
    MatchSubject::Synthetic { display_subject_range, .. } => (display_subject_range, Some(display_subject_range)),
    _ => (x.subject.range(), None)
};

would make this more explicit and hence easier to follow. (This would also make the display_subject_range function unused.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oops, I pressed the wrong button. This could also turn into a let display_subject_range = match_subject.display_subject_range(); before the insert and

subject_range: display_subject_range.unwrap_or_else(|| x.subject.range()),
display_subject_range,

matches!(self, MatchSubject::Synthetic { .. })
}

fn display_subject_range(&self, fallback: TextRange) -> TextRange {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Returning an Option<TextRange> and leaving it to the caller to determine what they want to do with a None seems more useful. This will also simplify some code below.

@lennardwalter

Copy link
Copy Markdown
Author

Thanks! Will gladly change this, however first would like to make sure that the solution with the maybe slightly unclean sentinel is acceptable. @hurryabit

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.

match funccall() ... loses exhaustiveness / reports missing return**

2 participants