Skip to content
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

Explorer: Support abstract virtual methods #2512

Closed
Pixep opened this issue Jan 5, 2023 · 19 comments · Fixed by #3411
Closed

Explorer: Support abstract virtual methods #2512

Pixep opened this issue Jan 5, 2023 · 19 comments · Fixed by #3411
Labels
explorer Action items related to Carbon explorer code good first issue Possibly a good first issue for newcomers

Comments

@Pixep
Copy link
Contributor

Pixep commented Jan 5, 2023

Background

The Carbon explorer currently supports virtual and impl prefixed virtual method, but not abstract methods (i.e. pure virtual methods in C++).

Goal

Support abstract method, as described in the class design doc. This includes:

  • Checking that the method has no implementation (CallableDeclaration::body())
  • Checking that child classes do implement the method

Starting point

A recommended approach would be to modify the switch/case logic for virtual methods declaration in explorer/interpreter/type_checker.cpp (in the TypeChecker::DeclareClassDeclaration function, VirtualOverride::Abstract case) to remove the current error logged when abstract is used, and check if the method has no body first.

This can be tested by creating a running a new lit test under explorer/testdata/class, and run using bazel run //explorer/testdata:class/<my_abstract_method_test.carbon>.run.

Example of tests:

Error case 1

base class A {
  // Error, abstract method cannot have a body (note the curly braces)
  abstract fn Foo[self: Self]() {}
}

Valid case

abstract class A {
  // OK, no implementation
  abstract fn Foo[self: Self]();
}

class B extends A {
  // OK, implemented in child class
  fn Foo[self: Self]() {}
}

Resources

@Pixep Pixep added explorer Action items related to Carbon explorer code good first issue Possibly a good first issue for newcomers labels Jan 5, 2023
@aphsai
Copy link

aphsai commented Jan 6, 2023

hi, could I take a crack at this?

@Pixep
Copy link
Contributor Author

Pixep commented Jan 6, 2023

hi, could I take a crack at this?

Welcome @aphsai ! We have another person on the Discord who started working on it yesterday (I will ask them to add a little note here to be assigned).
Feel free to join us there, and I will start thinking of another other good first issue as well.

@aphsai
Copy link

aphsai commented Jan 6, 2023

Thanks for the quick reply, happy to help if they need it.

@n3r0bi0m4n
Copy link

@Pixep here I am

@CODER2810
Copy link

HELLO I CAN RESOLVE THIS ISSUE KINDLY LET ME WORK IN THIS PROJECT

@ashutosh887
Copy link

HI @Pixep I would like to wrap this issue
Please assign it to me

@Pixep
Copy link
Contributor Author

Pixep commented Feb 27, 2023

Thanks for the enthusiasm, let's first see if @n3r0bi0m4n can finish it :)
@n3r0bi0m4n is it something you are still working on?

@CODER2810

This comment was marked as off-topic.

@ashutosh887

This comment was marked as off-topic.

@CODER2810

This comment was marked as off-topic.

@Pixep
Copy link
Contributor Author

Pixep commented Feb 28, 2023

Hid a few comments, to make sure we stay on topic. We may open the issue to be re-assigned in the future, but for now someone already is.

@shreya024
Copy link

Hid a few comments, to make sure we stay on topic. We may open the issue to be re-assigned in the future, but for now someone already is.

Hi @Pixep I would like to work on this issue if it is open

@Pixep
Copy link
Contributor Author

Pixep commented Mar 26, 2023

Hey everyone, good first issues don't need to be assigned anymore (see https://github.com/carbon-language/carbon-lang/blob/trunk/CONTRIBUTING.md#implement-carbons-design for some explanation), feel free to work on it if you wish!

@AbdullahGhulamNabi
Copy link

I'm eager to contribute to the effort to resolve the pure virtual function error problem as a student and to do so in order to improve the community while learning essential software development skills. Can I have a chance to work upon?

@himanshusingh0905
Copy link

@Pixep There is no method that can tell if a method has a body or not.

@Pixep
Copy link
Contributor Author

Pixep commented Apr 6, 2023

@Pixep There is no method that can tell if a method has a body or not.

During type checking, you can use fun->body().has_value(). You can see it in the reference work that was done on the same issue for an example here: #2528.

@shreya024 , @AbdullahGhulamNabi , @himanshusingh0905 the issue is open for anyone to work on it, without being assigned :)

@Jayantbhoj
Copy link

is it still open can i contribute?

@AbdullahGhulamNabi
Copy link

AbdullahGhulamNabi commented Sep 14, 2023 via email

@rishitha1238
Copy link

Hi Pixep, I would like to work on this if it's open

github-merge-queue bot pushed a commit that referenced this issue Nov 21, 2023
Completing the work from #2761 by adding additional test cases. 
Closes #2512

---------

Co-authored-by: Maan2003 <[email protected]>
Co-authored-by: Aleksei Ermakov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code good first issue Possibly a good first issue for newcomers
Projects
None yet
10 participants