Implement non-POD field access

[darichey]

Hi, this is some work on implementing field access for non-POD types. The approach is based on the ideas from #53 (roughly, it seems a lot has changed since that was written). We get in early (immediately after parsing) in the analysis and insert synthetic C++ methods for accessing the fields.

It's pretty rough, but I wanted to get some early feedback on the general approach, and ask about a few issues. In particular, I'm a bit stumped by a problem presented by the test_non_pod_with_non_copyable_member test:

struct Foo {
  Foo(const Foo &foo) = delete;
  int x;
};
struct Bar {
  Foo foo;
};

With these changes, we will attempt to insert an accessor like:

Foo Bar_get_foo(const Bar &bar) {
  return bar.foo;
}

This will fail, because we cannot copy foo. What should we do here? This might be related to my next point.

I was also considering the fact that in C++ we have several options for accessing a field, for example:

struct Baz {
  int x;
}

Baz baz{};
int x1 = baz.x;
int* x2 = &baz.x;
int& x3 = baz.x;

Is there a nice way we could support all of these?

I would greatly appreciate any feedback. Thanks!

[google-cla[bot]]

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]

Thanks a lot for raising this and figuring out where in the codebase to put everything. I am currently away for a while so I might not have a chance to do a thorough review straight away. But meanwhile I am excited you've done this. I have some ideas for the problems you've encountered but they are probably too nuanced for me to type on my phone, so expect a proper reply in a while. Thanks again!

[adetaylor]

General comments:

• There are some test failures that you should look into. #1134 is the only known test failure on main right now, so all the others are a consequence of this change.
• Thanks for doing the tedious CLA thing
• Regarding non-copyable things. Options:
  • (simpler) Initially provide accessors only for POD types. You should be able to look up which types are POD after we've called analyze_pod_apis which is pretty early in the analysis. Later than you're currently doing it, but not much later. (There are various existing bits of code which enumerate the POD types - see for example build_pod_safe_type_set)
  • (more complex) Add these accessors later once we've identified which types have SpecialMemberKind::CopyConstructor and friends. That's much later in the analysis, and would be a more ambitious change by far.
• Getting references/pointers to fields - my first instinct is not to do it as part of this PR but do it later (incremental changes FTW). But to avoid future naming conflicts, we should imagine what such future accessors might be called. I think we'd probably call them get_foo_ref or get_ref_foo (same with ptr) and thus it's OK if the current accessors are called get_foo

Thanks again for having a crack at this!

[darichey]

Thanks a lot for the feedback. I believe I have addressed all of it!

There are some test failures that you should look into

These were all related to non-copyable things (and possibly arrays) which is now fixed (see below)

Regarding non-copyable things

I've opted for the simpler option for now. As you said, incremental changes FTW :) I think the main thing that needs addressing with the current solution is the duplication of build_pod_safe_type_set. I wasn't quite sure where to move it to. Do you have any suggestions?

Getting references/pointers to fields - my first instinct is not to do it as part of this PR but do it later

That sounds reasonable!

[adetaylor]

the duplication of build_pod_safe_type_set

Good question. How's about a new api_utils.rs alongside api.rs?

[adetaylor]

This is looking pretty good now!

[drozdziak1]

Is there a reason not to merge this? (other than the windows failure)

[adetaylor]

That, plus it's still marked draft, it has a merge conflict (probably very easy to resolve) and there are still comments above that are not yet marked resolved. But I do want to merge this ASAP!

[darichey]

Hi, sorry for the delay here. Unfortunately I don't have the time to continue with this PR. If anyone would like to pick it up and get it over the finish line, please feel free!

[silvanshade]

I was able to update this PR branch with the suggested changes in a local fork of mine a little while ago and would be willing to submit a follow up PR with those fixes sometime soon.