Allow constructing subclass of abstract class #1206

russelltg · 2023-01-15T20:38:20Z

Fixes #774

I have a solid start here, but I think I'm going to need some guidance to get this complete. I have it all working except overloads, and I need to somehow get the C++ types of the arguments from an Api<FnPrePhase2>

I put in a placeholder args: Vec<QualifiedName> in my Signature object but I'm not seeing anything in Function that I could use to fill that. Also, ideally the arguments wouldn't be the QualifiedName so type aliases don't get it confused

Tests pass
Appropriate changes to README are included in PR

google-cla · 2023-01-15T20:38:24Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

adetaylor · 2023-01-16T12:20:05Z

engine/src/conversion/analysis/abstract_types.rs

@@ -6,106 +6,209 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+use std::collections::HashMap;


Please could you use indexmap::map::IndexMap as HashMap so that we get predictable iteration order.

adetaylor · 2023-01-16T12:31:45Z

Thanks for having a stab at this! Looks to be along the right lines, and good job figuring out the convoluted codebase well enough to get this in the right places.

I need to somehow get the C++ types of the arguments from an Api

FnAnalysis::params is what you need. This contains the Rust types, but they're "converted", which means we have things like UniquePtr<Foo> rather than whatever bindgen gave us. Therefore the syn::Types in that field are essentially canonical and can be relied upon to uniquely represent C++ types; they should be good enough for overload resolution.
i.e. Rather than compare QualifiedNames, you should compare syn::Types, which is what you'll find in this location.

I'm not going to take a more detailed look at this until the tests are green and you tell me that it's good to go. Good luck!

russelltg · 2023-01-18T04:35:29Z

Thanks for the help there, I've figured out the overload resolution stuff.

The only missing thing is it doesn't really support pure virtual dtors in either the base or the sub class. I don't think pure virtual dtors get passed to the analysis? Or at least I don't see it.

russelltg · 2023-01-18T04:45:07Z

I'm going to go ahead and mark this as ready for review, as I think it's useful and correct as is. I'm happy to deal with the pure virtual dtor edgecase if you think that's important/worthwhile.

adetaylor · 2023-01-19T15:11:55Z

Thanks, I'm travelling right now so I'm unlikely to take a look at this for a few days, but I'll get there.

adetaylor

Sorry for the delay on reviewing this.

It's looking great!

My comments are:

a load of nit picks and suggestions
one substantial comment that I can't easily follow what happens in cases where our data structures are incomplete (for unforeseen reasons) - I want to make sure we're erring on the side of regarding classes as abstract. I'm sure I could figure it out by peering more carefully at it, but the fact it's hard to follow suggests it could do with a little bit more in the way of comments.

adetaylor · 2023-01-29T08:26:48Z

engine/src/conversion/analysis/abstract_types.rs

+                        kind:
+                            FnKind::Method {
+                                impl_for: self_ty_name,
+                                method_kind: MethodKind::Virtual(constness),


Please could you reduce the duplication of code here by just matching as far as FnKind::Method in the outer match, and then having an additional match on the method_kind to choose between defined and undefined.

adetaylor · 2023-01-29T08:30:42Z

engine/src/conversion/analysis/abstract_types.rs

-                        Api::Struct {
-                            analysis: PodAndConstructorAnalysis {
-                                pod: PodAnalysis {
+        for api in apis.iter() {


Please could you look into whether fields_and_bases_first can simplify this code? This is an iterator through the APIs in a specific order which should remove the need for the outer while loop.

I think fields_and_bases_first is newer than the original code in this function, so that's why the code didn't use it originally. As you're reworking this function anyway, it would be good to see if you can switch to this at the same time?

If it doesn't work out - that's fine. (But please could you add a comment saying why not?)

adetaylor · 2023-01-29T08:31:51Z

engine/src/conversion/analysis/abstract_types.rs

+
+                    let state = class_states.entry(name.name.clone()).or_default();
+
+                    newset = newset


Can you do newset.retain(...)?

adetaylor · 2023-01-29T08:43:01Z

engine/src/conversion/analysis/abstract_types.rs

+                    class_states
+                        .get(b)
+                        .map(|cs| !cs.undefined.is_empty())
+                        .unwrap_or(false)


There are a lot of negatives in these two lines (!, undefined, is_empty, false) so it's not too easy to read. Could you add a comment?

I'm trying to convince myself that unwrap_or(false) is correct. Goal 1 is to avoid generating faulty bindings that will prevent autocxx working with any codebase, so if we're unsure we should err on the side of regarding a class as abstract. I'm not able to convince myself whether false is right for that. Under what circumstances would a class state not be recorded in the maps?

I've now been through the whole thing and this turns out to be my only substantial comment. I just want to be 100% sure we're erring on the side of caution if there are unforeseen circumstances which might result in our data structures being incomplete.

adetaylor · 2023-01-29T08:44:46Z

engine/src/conversion/analysis/abstract_types.rs

-            })
-            .collect()
+
+                _ => {}


This could be if let instead, now you're no longer doing anything with the other cases

adetaylor · 2023-01-29T08:52:27Z

integration-tests/tests/integration_test.rs

+
+    "};
+    let rs = quote! {
+        static_assertions::assert_impl_all!(ffi::FullyDefined: moveit::CopyNew);


I wish I'd thought of using this for loads of the other tests!

adetaylor · 2023-01-29T08:56:24Z

engine/src/conversion/analysis/abstract_types.rs

+                            PodAnalysis {
+                                bases,
+                                kind: TypeKind::Pod | TypeKind::NonPod,
+                                castable_bases,


I know you just copied the old code pattern here, but I think it's got more and more yucky as more fields have gradually accumulated in PodAnalysis. If you want to simplify this to something like:

for api in apis.iter_mut() { if let Api::Struct(api) = api { if api is abstract { api.blah.blah.blah.kind = TypeKind::Abstract } } }

(or something like that) that would be fine with me. But also, feel free not.

adetaylor · 2023-01-29T08:57:19Z

engine/src/conversion/analysis/abstract_types.rs

+                        name: name.cpp_name(),
+                        args: params
+                            .iter()
+                            .skip(1)


Please add comment // skip receiver or somesuch

adetaylor · 2023-01-29T09:00:01Z

integration-tests/tests/integration_test.rs

+
+        // class Partial5 : public Base {
+        // public:
+        //     ~Partial5() = 0;


Totally happy not to address this case now, but please could you add a comment in this test case explaining why this is commented out.

(also if you could file a new github issue that'd be great)

russelltg · 2023-01-30T04:20:54Z

fields_and_bases_first is super cool, thanks for pointing me there, that significnatly simplifies the code. The only problem....it doesn't seem to do methods first. It's definitely emitting the Struct api's before the methods.....

I suppose the name is evident--it's fields and bases first not fields, methods, and bases first. I think the cleanest way to fix this would be to either have a new traverser that does to methods first (or just edit fields_and_bases_first to do that) or just go through the API list twice like I was doing previously

adetaylor · 2023-01-30T10:13:01Z

I suppose the name is evident--it's fields and bases first not fields, methods, and bases first. I think the cleanest way to fix this would be to either have a new traverser that does to methods first (or just edit fields_and_bases_first to do that)

Originally it did do this, but it turned out this caused cycles, because (I think) some types of constructor need to depend on the type they're constructing. You'd be most welcome to try to solve this but I think it was Hard. (I can't remember the details). So going through the list twice is probably best.

adetaylor · 2023-01-30T10:16:28Z

engine/src/conversion/apivec.rs

@@ -97,6 +97,10 @@ impl<P: AnalysisPhase> ApiVec<P> {
        self.apis.iter()
    }

+    pub(crate) fn iter_mut(&mut self) -> impl Iterator<Item = &mut Api<P>> {
+        self.apis.iter_mut()


Oh, I'm really sorry, I offered you bad advice here. iter_mut is considered harmful here for the reasons explained in this comment:
https://github.com/google/autocxx/pull/1206/files/8d4d785f4105c3332b1f77f8f0c0756ef9e72545..c90cd896eca376977c87e04707ec48ba01b243bd#diff-dd61892ff96d055fbcab83c5eecc76c8b6120c3eb2f1b5687a8c025435200fcdL32
I just plain forgot! Sorry for the rework.

adetaylor · 2023-01-30T10:18:43Z

engine/src/conversion/analysis/abstract_types.rs

+    let mut abstract_classes = HashSet::new();
+
+    for api in fields_and_bases_first(apis.iter()) {
+        match dbg!(api) {


Don't forget to remove this dbg

russelltg · 2023-02-09T17:56:57Z

This is ready for review @adetaylor :)

adetaylor · 2023-02-17T16:09:35Z

I'm good to merge this once the formatting problems are fixed!

adetaylor · 2023-02-20T10:13:44Z

Thanks very much for this!

russelltg added 2 commits January 15, 2023 18:01

get started

910e7b5

some more work

d2aa277

adetaylor reviewed Jan 16, 2023

View reviewed changes

russelltg added 2 commits January 17, 2023 22:31

fix up overload resolution

f6829c2

listen to clippy

12bf667

russelltg marked this pull request as ready for review January 18, 2023 04:45

russelltg added 3 commits January 21, 2023 10:46

add missing trait

38f26ef

fmt

e1d172f

clippy

8d4d785

adetaylor requested changes Jan 29, 2023

View reviewed changes

address some comments

c90cd89

adetaylor reviewed Jan 30, 2023

View reviewed changes

russelltg added 2 commits February 4, 2023 21:09

go through APIs one final time to go through methods before classes

0b46f92

Merge remote-tracking branch 'origin/main' into construct_concrete_class

8a5e115

fmt

a303a9a

adetaylor merged commit 5540b86 into google:main Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow constructing subclass of abstract class #1206

Allow constructing subclass of abstract class #1206

russelltg commented Jan 15, 2023 •

edited

google-cla bot commented Jan 15, 2023

adetaylor Jan 16, 2023

adetaylor commented Jan 16, 2023

russelltg commented Jan 18, 2023 •

edited

russelltg commented Jan 18, 2023

adetaylor commented Jan 19, 2023

adetaylor left a comment

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

adetaylor Jan 29, 2023

russelltg commented Jan 30, 2023 •

edited

adetaylor commented Jan 30, 2023

adetaylor Jan 30, 2023

adetaylor Jan 30, 2023

russelltg commented Feb 9, 2023

adetaylor commented Feb 17, 2023

adetaylor commented Feb 20, 2023


		let state = class_states.entry(name.name.clone()).or_default();

		newset = newset

Allow constructing subclass of abstract class #1206

Allow constructing subclass of abstract class #1206

Conversation

russelltg commented Jan 15, 2023 • edited

google-cla bot commented Jan 15, 2023

Choose a reason for hiding this comment

adetaylor commented Jan 16, 2023

russelltg commented Jan 18, 2023 • edited

russelltg commented Jan 18, 2023

adetaylor commented Jan 19, 2023

adetaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russelltg commented Jan 30, 2023 • edited

adetaylor commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russelltg commented Feb 9, 2023

adetaylor commented Feb 17, 2023

adetaylor commented Feb 20, 2023

russelltg commented Jan 15, 2023 •

edited

russelltg commented Jan 18, 2023 •

edited

russelltg commented Jan 30, 2023 •

edited