Throws Bad Instantation Error when Creating an Instance of an Abstract Class

[Adist319]

Resolves #592.

We now only throw an error when creating an instance of an abstract class, instead of when an abstract method is called.

Behavior:

# Before: No error thrown

# After: BadInstantiation error
class Shape(ABC):
    @abstractmethod
    def area(self): pass

Shape()  # Error: Cannot instantiate abstract class `Shape`

# Concrete implementations work fine
class Circle(Shape):
    def area(self): return 3.14

Circle()  # works

[yangdanny97]

Hmm, mypy primer runs are panicking. We'll get this reviewed and see if we can figure out what's going on

[yangdanny97]

this appears unused?

[yangdanny97]

I think we can pre-calculate inherits_from_abc and store it in class metadata. cc @grievejia

[Adist319]

I like this idea, we could do something like:

pub struct ClassMetadata {
       // ... existing fields ...
       inherits_from_abc: bool,  // New field
}

And then we could populate ABC Inheritance during class metadata creation.

[yangdanny97]

we might need to handle overloaded functions too

[yangdanny97]

I'm not sure we need to traverse all the base classes like this. If we look up each field the class has like so

pyrefly/pyrefly/lib/alt/class/class_field.rs

Line 1589 in b35b7ab

fn get_class_member_impl(

it would handle the lookup, and we can check if the field it returns is abstract or not.

Of course, this relies on the MRO being correct and able to handle cases when one base class declares something as abstract and another base class provides the impl. @stroxler would know more about this

[Adist319]

@yangdanny97 Thanks for the feedback! I just wanted to check if the panics have been root-caused yet, or if that’s still being investigated, so I know when I can continue work on this PR.

[facebook-github-bot]

@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D80087011.

[yangdanny97]

@Adist319

The panics are caused by the changes this PR, probably to handling descendants of the PathLike class (that's what declares the __fspath__ we see in the panic messages.

You can repro this by checking a simple snippet.

from pathlib import Path
p = Path("x")

This didn't show up in the unit tests, because we emit a DEBUG message instead of panic:

pyrefly/pyrefly/lib/state/state.rs

Line 1517 in 29aa811

"LookupAnswer::get failed to find key, {module} {k:?} (concurrent changes?)"

cc @ndmitchell I'm a bit confused about what this does, or why we don't always panic

[Adist319]

@yangdanny97 thanks for looking into it , will get back to this soon.

[Adist319]

@yangdanny97 Apologies for the extended delay, but I think I've made good progress here.
I've implemented abstract class instantiation checking for #592 and #511 - abstract classes can't be instantiated and @Final validation works. But I ran into a problem:
3 pre-existing tests are now failing:

• test::decorators::test_abstract_classmethod
• test::decorators::test_abstract_staticmethod
• test::descriptors::test_abstract_property

These test deprecated Python decorators (@abstractclassmethod, @abstractstaticmethod, @abstractproperty - deprecated 10+ years ago I believe?).

Current behavior:

• Abstract checking works (can't instantiate)
• Type resolution returns Any instead of proper type

class C(abc.ABC):
    @abc.abstractclassmethod
    def f(cls) -> int:
        return 42

assert_type(C.f(), int)  # FAILS: Got Any, expected int

Question

Should I fix type resolution for these deprecated decorators or can we skip these 3 tests?

I can create a new pull request once all tests pass.

[yangdanny97]

Thanks for continuing to work on this! Do you mind updating the PR so I can pull the changes and dig a bit into what's happening?

Given that we're returning Any instead of emitting a spurious error, I'm tentatively OK with skipping those tests as long as the new version decorated w/ both @abstract @classmethod behaves correctly.

[Adist319]

New PR #1185 for this, trying to resolve a delimiter issue after resolving some merge conflicts. But feel free to take a look at the changes first.