Drake: spatial_algebra: `Class::dot` implementations cause dependency cycles

[EricCousineau-TRI]

From merge-base 9b7cae4c56e, when writing bindings for some of the SpatialVector derived classes, I got the following runtime error when running the Python unittest:

    from pydrake.multibody.math import (                                                               
ImportError: ...runfiles/drake/bindings/pydrake/multibody/math.so: undefined symbol: _ZNK5drake9multibody15SpatialVelocityIdE3dotERKNS0_12SpatialForceIdEE

Looking into it, it's because the ::dot definitions are defined in spatial_algebra.h???
https://github.com/RobotLocomotion/drake/blob/19a15cd364efe6ef8d23ece115375948b546810c/multibody/math/spatial_algebra.h

EDIT (2020-09-22):

Two issues:

• [x] #14113 The symbols are not compiled into //:drake_shared_library
• [ ] The dependency cycles may mean that partial includes could happen (compile time) or some symbols may not be linked (linking time)

FYI @jwnimmer-tri

[EricCousineau-TRI]

As a minor workaround, I'll just include spatial_algebra.h in the binding file itself.

[jwnimmer-tri]

I agree that this violates GSG's rule about self-contained headers.

I don't think it is "configuration: python", though -- this is a problem for C++ users as well.

[SeanCurtis-TRI]

@amcastro-tri As platform reviewer of the day, I'm assigning this to you. I'll defer to you if it should be passed along.

[amcastro-tri]

Ideally I'd like those inline. What should be done here? place explicit instantiations by hand in a source file?

[jwnimmer-tri]

It's a dependency cycle, not easily cured. Ask @rpoyner-tri to help talk you through it.

With only a quick look, to me the only solution is to present a single header for all users (even internal users) to include, for the whole package. That's the only way you can be sure that all callers receive the inline method bodies -- all callers must have all headers fully included. The explicit instantiations would only help with linking, not inlining.

[EricCousineau-TRI]

This issue is specifically about linking from //:drake_shared_library, so just adding them in something like spatial_algebra.cc would resolve this issue (IMO).

If y'all feel that the scope should be broadened to all linking cases, then should we expand the scope? (I can edit the title to reflect that if appropriate.)

[jwnimmer-tri]

... not easily cured.

Or maybe... it might be enough to add a simple #include "spatial_algebra.h" to spatial_{force,momentum,velocity}.cc along with adding that header to srcs for those rules. Then the definition is available at the time of explicit template instantiation.

That would at least fix the problems with unmet linking promises. The inlining design is still broken, though.

[rpoyner-tri]

Had an initial look. From a physical-design POV, ewww. From the GSG perspective, the code flouts advice about forward declarations. From a module dependency perspective (module being a .cc/.h pair, roughly), there are dependency cycles for each dot() method.

Looks like we want to preserve the X.dot(Y) syntax to imitate an infix operator, and have the client sites look somewhat notationally pleasing. If this were not the case, we could just have global/namespace functions dot(X, Y), and get on with our lives. FWIW, there are something like ~400 call sites in Drake itself, and probably plenty in Drake clients.

I'll see what I can propose to repair this.

[rpoyner-tri]

My dev branch here https://github.com/rpoyner-tri/drake/tree/spaghetti-algebra has a proposal that breaks the dependency cycles. I haven't checked to see if inlining works in representative cases yet. Also, the documentation probably needs a lot more work. Shout-outs to interested parties not yet seen in this issue: @sherm1 @mitiguy

[EricCousineau-TRI]

Hm... I think that solution (ensure all declarations are present in one bundle) goes beyond the scope of this issue (make sure stuff is present in //:drake_shared_library).

I'll expand the scope, but FWIW I'd be happy w/ Jeremy's solution for now.

EDIT: Done.

[EricCousineau-TRI]

@rpoyner-tri I think your current approach may still hurt users if headers aren't bundled together.

e.g. if I don't include spatial_algebra.h, then my linker error now turns into a DotProductNotPermitted, and now I'm scratching my head why a valid dot product is explicitly disabled.

I think the most robust solution (in this case) would be to either bundle the headers (as is done with symbolic), or perhaps just expand the error behavior to be sthg like

static_assert(deferred_false<T>, "This may not be a valid combination. Please ensure you have included `spatial_algebra.h` and are using the right types)

However, that still has a funny smell to it...

(And also - thank you for looking into it!)

[EricCousineau-TRI]

I've filed one possible implementation of Jeremy's suggestion: #14113

[sherm1]

My dev branch here https://github.com/rpoyner-tri/drake/tree/spaghetti-algebra has a proposal that breaks the dependency cycles.

Clever! That looks like it would inline nicely. I do worry that the switch from ordinary function to many-layered template meta function could produce some horrifying error messages.

[jwnimmer-tri]

FYI I'm with @EricCousineau-TRI on this one. The headers are all super tiny; the compile time cost of parsing all of them is unlikely to matter. Thus, they should be (effectively) a single header. That would fix all of the forward-decl, inlining, and explicit template instantiation problems in one fell swoop, with low complexity.

[EricCousineau-TRI]

I think #14113 could close this issue out without needing to resort to symbolic.h-style gluing -- yes, it does have forward declarations, so users would get a weird error message if they try to construct say a SpatialForce if they only included spatial_vector.h, but if the user includes spatial_force.h, then they'd be good.

Is it super important to put those under one roof? (I'm kinda ambivalent, but just checking on the severity of the forward-decl failure mode)

[jwnimmer-tri]

The current component design is an abuse of C++ and violates the GSG. I think we should have an issue or TODO to track that, or at least a disclaimer in the code not to replicate the pattern. I don't care if it's under this issue number's umbrella or not.

The most important consequence of the abuse is that it's _way too easy_ to accidentally end up with out-of-line dot products simply by forgetting to include the spatial_algebra.h header. Neither the compiler nor linker will complain, but every dot will be a out-of-line function call instead of inlined, even though to a naive reading none of the methods are in cc files so should be expected to inline.

[rpoyner-tri]

FTR, my branch above still has the module slicing defect. It just converts a link error to a compiler error at considerable cost in source complexity.

[EricCousineau-TRI]

Given merge of #14113, do we still think this issue is useful as it's scoped?

IMO, nah, not really in the grand scheme of things (for my purposes).

[jwnimmer-tri]

@EricCousineau-TRI you can just unsubscribe. I care about fixing this code, so I'll take it over.

[jwnimmer-tri]

For the record, I think the bug was introduced as of #5365.

[jwnimmer-tri]

My proposed solution will be:

• deprecate direct inclusion of the headers other than spatial_algebra.h
• deprecate direct use of the cc_library names than :spatial_algebra
• consolidate the cc files into just a single one spatial_algebra.cc