Skip to content

fix: don't panic on a non-generic classproperty in get_attributes - #4457

Closed
renz011tzar wants to merge 1 commit into
facebook:mainfrom
renz011tzar:fix-classproperty-no-targs
Closed

fix: don't panic on a non-generic classproperty in get_attributes#4457
renz011tzar wants to merge 1 commit into
facebook:mainfrom
renz011tzar:fix-classproperty-no-targs

Conversation

@renz011tzar

Copy link
Copy Markdown
Contributor

Fixes #4453.

get_kind_and_field_type matched classproperty / cached_classproperty on the class name alone, then unwrapped the first type argument:

Type::ClassType(c)
    if c.name() == "classproperty" || c.name() == "cached_classproperty" =>
{
    let result_ty = c.targs().as_slice().first().unwrap();

A class with that name but no type arguments reaches this arm with empty targs, so get_attributes panics.

Change

Per @yangdanny97's suggestion in the issue, the no-type-arguments case now behaves as (None, ty). Rather than adding a nested match, the check moves into the match guard, so such a class simply doesn't match this arm and falls through to the existing _ => (None, ty):

Type::ClassType(c)
    if (c.name() == "classproperty" || c.name() == "cached_classproperty")
        && let Some(result_ty) = c.targs().as_slice().first() =>
{
    (Some(String::from("property")), result_ty)
}

Testing

Adds test_get_attributes_non_generic_classproperty, following the existing TempDir + create_query() pattern in test/query.rs. It checks out a module defining a plain class classproperty: used as a decorator, and asserts get_attributes returns the attribute rather than panicking.

  • The test fails without the fix (panicked at pyrefly/lib/query.rs:1147:66: called Option::unwrap() on a None value) and passes with it.
  • cargo test -p pyrefly --lib test::query — 7 passed, 0 failed.
  • cargo fmt --check clean.

@meta-cla

meta-cla Bot commented Aug 5, 2026

Copy link
Copy Markdown

Hi @renz011tzar!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@github-actions github-actions Bot added the size/s label Aug 5, 2026
@renz011tzar renz011tzar closed this Aug 5, 2026
@renz011tzar renz011tzar reopened this Aug 5, 2026
@meta-cla

meta-cla Bot commented Aug 5, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the cla signed label Aug 5, 2026
@github-actions github-actions Bot added size/s and removed size/s labels Aug 5, 2026
get_kind_and_field_type matched `classproperty`/`cached_classproperty` on
the class name alone and then unwrapped the first type argument. A class with
that name but no type arguments reached the arm with empty targs, so
get_attributes panicked.

Move the type-argument check into the match guard so such a class falls
through to the existing `_ => (None, ty)` arm instead.

Fixes facebook#4453
@renz011tzar
renz011tzar force-pushed the fix-classproperty-no-targs branch from 266c222 to 417ce55 Compare August 5, 2026 06:31
@github-actions github-actions Bot added size/s and removed size/s labels Aug 5, 2026
@meta-codesync

meta-codesync Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D114848162. (Because this pull request was imported automatically, there will not be any future comments.)

@yangdanny97 yangdanny97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

@meta-codesync meta-codesync Bot closed this in 1ba9829 Aug 5, 2026
@meta-codesync

meta-codesync Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@ndmitchell merged this pull request in 1ba9829.

@meta-codesync meta-codesync Bot added the Merged label Aug 5, 2026
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.

panic: Option::unwrap() on None in Query::get_attributes for a non-generic classproperty class

3 participants