-
Notifications
You must be signed in to change notification settings - Fork 145
Throws Bad Instantation Error when Creating an Instance of an Abstract Class #807
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -1782,4 +1782,117 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { | |||
.and_then(|ty| make_bound_method(Type::type_form(cls.clone().to_type()), ty).ok()) | ||||
} | ||||
} | ||||
|
||||
/// Check if a class inherits from abc.ABC | ||||
/// | ||||
/// This recursively traverses the class hierarchy to determine if the class | ||||
/// inherits from Python's abstract base class (abc.ABC). Only classes that | ||||
/// inherit from abc.ABC can be considered abstract in Python. | ||||
fn inherits_from_abc(&self, cls: &ClassType) -> bool { | ||||
let class_obj = cls.class_object(); | ||||
|
||||
// Check if this class is abc.ABC itself | ||||
if class_obj.module_name().as_str() == "abc" && class_obj.name().as_str() == "ABC" { | ||||
return true; | ||||
} | ||||
|
||||
// Recursively check base classes | ||||
let metadata = self.get_metadata_for_class(class_obj); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can pre-calculate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this idea, we could do something like:
And then we could populate ABC Inheritance during class metadata creation. |
||||
for base in metadata.bases_with_metadata() { | ||||
if self.inherits_from_abc(&base.0) { | ||||
return true; | ||||
} | ||||
} | ||||
|
||||
false | ||||
} | ||||
|
||||
/// Check if a class has any unimplemented abstract methods | ||||
/// | ||||
/// Returns true if the class: | ||||
/// 1. Inherits from abc.ABC (or is abc.ABC itself) | ||||
/// 2. Has abstract methods in its hierarchy that are not implemented | ||||
/// | ||||
/// This method is used to prevent instantiation of abstract classes. | ||||
pub fn has_abstract_methods(&self, cls: &ClassType) -> bool { | ||||
// Only classes that inherit from abc.ABC can be abstract | ||||
if !self.inherits_from_abc(cls) { | ||||
return false; | ||||
} | ||||
|
||||
// Check for unimplemented abstract methods with early exit | ||||
self.has_unimplemented_abstract_methods(cls) | ||||
} | ||||
|
||||
/// Check if a class has unimplemented abstract methods | ||||
fn has_unimplemented_abstract_methods(&self, cls: &ClassType) -> bool { | ||||
let mut abstract_methods = SmallSet::new(); | ||||
|
||||
// Collect all abstract methods from the hierarchy | ||||
self.collect_abstract_methods_from_class(cls, &mut abstract_methods); | ||||
|
||||
// Check each abstract method for concrete implementation | ||||
for method_name in &abstract_methods { | ||||
if !self.class_provides_concrete_implementation(cls, method_name) { | ||||
return true; | ||||
} | ||||
} | ||||
|
||||
false | ||||
} | ||||
|
||||
/// Recursively collect abstract methods from a class and its bases | ||||
fn collect_abstract_methods_from_class( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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 |
||||
&self, | ||||
cls: &ClassType, | ||||
abstract_methods: &mut SmallSet<Name>, | ||||
) { | ||||
let class_obj = cls.class_object(); | ||||
let metadata = self.get_metadata_for_class(class_obj); | ||||
|
||||
// First collect from base classes | ||||
for base in metadata.bases_with_metadata() { | ||||
self.collect_abstract_methods_from_class(&base.0, abstract_methods); | ||||
} | ||||
|
||||
// Then collect abstract methods from this class | ||||
for field_name in class_obj.fields() { | ||||
if let Some(field) = self.get_from_class( | ||||
class_obj, | ||||
&KeyClassField(class_obj.index(), field_name.clone()), | ||||
) { | ||||
match &field.0 { | ||||
ClassFieldInner::Simple { ty, .. } => { | ||||
if let Type::Function(function) = ty { | ||||
if function.metadata.flags.is_abstract_method { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we might need to handle overloaded functions too |
||||
abstract_methods.insert(field_name.clone()); | ||||
} | ||||
} | ||||
} | ||||
} | ||||
} | ||||
} | ||||
} | ||||
|
||||
/// Check if a class provides a concrete (non-abstract) implementation of a method | ||||
fn class_provides_concrete_implementation(&self, cls: &ClassType, method_name: &Name) -> bool { | ||||
let class_obj = cls.class_object(); | ||||
|
||||
// Check if the class has this method | ||||
if let Some(field) = self.get_from_class( | ||||
class_obj, | ||||
&KeyClassField(class_obj.index(), method_name.clone()), | ||||
) { | ||||
match &field.0 { | ||||
ClassFieldInner::Simple { ty, .. } => { | ||||
if let Type::Function(function) = ty { | ||||
// Method exists - check if it's concrete (not abstract) | ||||
return !function.metadata.flags.is_abstract_method; | ||||
} | ||||
} | ||||
} | ||||
} | ||||
|
||||
false // Method not found or not a function | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/* | ||
* Copyright (c) Meta Platforms, Inc. and affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
use crate::testcase; | ||
|
||
testcase!( | ||
test_abstract_class_instantiation_error, | ||
r#" | ||
from abc import ABC, abstractmethod | ||
|
||
class Shape(ABC): | ||
@abstractmethod | ||
def area(self) -> float: | ||
pass | ||
|
||
# This should error - cannot instantiate abstract class | ||
shape = Shape() # E: Cannot instantiate abstract class `Shape` | ||
"#, | ||
); | ||
|
||
testcase!( | ||
test_concrete_subclass_instantiation_ok, | ||
r#" | ||
from abc import ABC, abstractmethod | ||
|
||
class Shape(ABC): | ||
@abstractmethod | ||
def area(self) -> float: | ||
pass | ||
|
||
class Circle(Shape): | ||
def area(self) -> float: | ||
return 3.14 | ||
|
||
# This should work - concrete subclass can be instantiated | ||
circle = Circle() | ||
"#, | ||
); | ||
|
||
testcase!( | ||
test_polymorphic_calls_ok, | ||
r#" | ||
from abc import ABC, abstractmethod | ||
|
||
class Shape(ABC): | ||
@abstractmethod | ||
def area(self) -> float: | ||
pass | ||
|
||
class Circle(Shape): | ||
def area(self) -> float: | ||
return 3.14 | ||
|
||
def calculate_area(shape: Shape) -> float: | ||
# This should work - polymorphic call is allowed | ||
return shape.area() | ||
|
||
circle = Circle() | ||
area = calculate_area(circle) | ||
"#, | ||
); | ||
|
||
testcase!( | ||
test_multiple_abstract_methods, | ||
r#" | ||
from abc import ABC, abstractmethod | ||
|
||
class Drawable(ABC): | ||
@abstractmethod | ||
def draw(self) -> None: | ||
pass | ||
|
||
@abstractmethod | ||
def erase(self) -> None: | ||
pass | ||
|
||
# This should error - class has multiple abstract methods | ||
drawable = Drawable() # E: Cannot instantiate abstract class `Drawable` | ||
"#, | ||
); | ||
|
||
testcase!( | ||
test_inherited_abstract_method, | ||
r#" | ||
from abc import ABC, abstractmethod | ||
|
||
class Base(ABC): | ||
@abstractmethod | ||
def method(self) -> None: | ||
pass | ||
|
||
class Child(Base): | ||
# Child doesn't implement method, so it's still abstract | ||
pass | ||
|
||
# This should error - child class is still abstract | ||
child = Child() # E: Cannot instantiate abstract class `Child` | ||
"#, | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
#![cfg(test)] | ||
|
||
mod abstract_class_instantiation; | ||
mod assign; | ||
mod attribute_narrow; | ||
mod attributes; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this appears unused?