use preprocessor guards to error include order problems

[bob-carpenter]

Summary:

From @jpritikin on the stan-dev group in response to a query about header include order causing problems:

Is there a way to catch this kind of thing with the C preprocessor? For example, a trait could #define I_AM_A_TRAIT and then if some other header got included later in the wrong order then you could #error.

Description:

We run into order dependencies in our header includes, primarily for traits. For example, we often require the Eigen version of a recursive template trait metaprogram to be included before the standard vector version in order ot handle arrays of matrices. We'd like to be able to catch wrong include orders at build time rather than getting perplexing errors.

Reproducible Steps:

A concrete example is including stan/math/prim/arr.hpp before stan/math/prim/mat.hpp.

Current Output:

Compliation failure.

Expected Output:

Prepropcoessor error with informative error message.

Current Version:

v2.11.0

[jpritikin]

Daniel Lee wrote:

prim x scal. lines 5-36:Â https://github.com/stan-dev/math/blob/develop/stan/math/prim/scal.hpp
prim x arr. lines 4-12, then it's really the prim x scal lines:Â https://github.com/stan-dev/math/blob/develop/stan/math/prim/arr.hpp
prim x mat. lines 4-21, then the lines from prim x arr, then prim x scal: https://github.com/stan-dev/math/blob/develop/stan/math/prim/mat.hpp
rev x scal. lines 4-7, then prim x scal. https://github.com/stan-dev/math/blob/develop/stan/math/rev/scal.hpp
rev x arr. lines 4-6, then prim x arr, then rev x scal. https://github.com/stan-dev/math/blob/develop/stan/math/rev/arr.hpp
rev x mat. lines 4-8, then prim x mat, then rev x arr. https://github.com/stan-dev/math/blob/develop/stan/math/rev/mat.hpp

I think Bob's thrown out the dependencies in the past. It looks like
something like this (for the first line prim doesn't depend on
anything, rev depends on prim):
prim <- rev
<- fwd
rev + fwd <- mix
scal <- arr <- mat

[jpritikin]

Okay so which include orders ought to generate errors? If I understand, it's an error to include any prim after rev, correct?

[syclik]

It's an error to include prim after rev. It's an error to include scal
after arr. It's an error to include scal after mat. It's an error to
include arr after mat.

Some of the traits are only defined as general template functions without
specializations in prim. Hopefully that doesn't overly complicate things.

On Fri, Sep 9, 2016 at 10:07 AM, Joshua Pritikin notifications@github.com
wrote:

Okay so which include orders ought to generate errors? If I understand,
it's an error to include any prim after rev, correct?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#383 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_FxVkIY6g4hCNZeLWPEn-T1XveXr_ks5qoWgjgaJpZM4J0gEg
.

[jpritikin]

It's an error to include prim after rev.

So for all these "it's an error to" relations, can you add a #define like STAN_REV and #error #ifdef STAN_REV in a prim context?

[bob-carpenter]

We need to include classes that instantiate templates
before the templates.

So including in these orders will cause problems:

mix < rev
mix < fwd
mix < prim
rev < prim
fwd < prim
fwd < rev

There's also redundancy, because

mix includes fwd
mix includes rev
mix includes prim
fwd includes prim
rev includes prim

So including the right hand side is just a redundant
header (there are header guards, so it's not like
it's a ton of I/O).

• Bob

On Sep 9, 2016, at 10:07 AM, Joshua Pritikin notifications@github.com wrote:

Okay so which include orders ought to generate errors? If I understand, it's an error to include any prim after rev, correct?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bob-carpenter]

Ah, yes, forgot the other direction. Error
to include any of the following, too:

prim < arr
prim < mat
arr < mat

We need a diagram.

• Bob

On Sep 9, 2016, at 10:12 AM, Daniel Lee notifications@github.com wrote:

It's an error to include prim after rev. It's an error to include scal
after arr. It's an error to include scal after mat. It's an error to
include arr after mat.

Some of the traits are only defined as general template functions without
specializations in prim. Hopefully that doesn't overly complicate things.

On Fri, Sep 9, 2016 at 10:07 AM, Joshua Pritikin notifications@github.com
wrote:

Okay so which include orders ought to generate errors? If I understand,
it's an error to include any prim after rev, correct?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#383 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_FxVkIY6g4hCNZeLWPEn-T1XveXr_ks5qoWgjgaJpZM4J0gEg
.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bob-carpenter]

Would it look like this in `prim/mat.hpp` ``` #ifdef STAN_REV_MAT #error MUST_USE_MIX_MAT_HPP_FOR_FWD_AND_REV #endif ``` given `rev/mat.hpp` has ``` #ifndef STAN_REV_MAT #define STAN_REV_MAT #endif ```

[syclik]

@jpritikin, you're working on this?

[jpritikin]

@syclik No, I'm not working on it. I didn't realize that it was my project. It just seemed like a good idea.

[syclik]

I'll assign it to you. I still don't know how to get it done. I think you have the best chance of doing it right. That ok?

[syclik]

(nm... I can't assign it to you.)

[syclik]

but if you know how to do it, it would help if you did build it out. Or at least showed us how.

[bob-carpenter]

See my comment above on what needs to be done. I'm buried in other projects right now, but could eventually do this at the coarse level. I think getting this right file-by-file would be trickier.

[syclik]

That wasn't enough bread crumbs for me to follow along. Are you thinking that #define happens at the end?

@jpritikin, if that makes sense to you, mind throwing up an example for one of the headers? I think if I saw an example, I'd probably be able to do something about it.

[alashworth]

Are the comments in this issue still up-to-date?

I was considering adding a rule to sort includes in .clang-format via IncludeCategories,

  - Regex:        '^["<]stan/math/prim/scal'
  - Priority: 101
  - Regex:        '^["<]stan/math/prim/arr'
  - Priority: 102
  - Regex:        '^["<]stan/math/prim/mat'
  - Priority: 103
  - Regex:        '^["<]stan/math/prim/.*'
  - Priority: 104
  - Regex:        '^["<]stan/math/fwd/scal'
  - Priority: 105
  - Regex:        '^["<]stan/math/fwd/arr'
  - Priority: 106
  - Regex:        '^["<]stan/math/fwd/mat'
  - Priority: 107
  - Regex:        '^["<]stan/math/fwd/.*'
  - Priority: 108
  - Regex:        '^["<]stan/math/rev/scal'
  - Priority: 109
  - Regex:        '^["<]stan/math/rev/arr'
  - Priority: 110
  - Regex:        '^["<]stan/math/rev/mat'
  - Priority: 111
  - Regex:        '^["<]stan/math/rev/.*'
  - Priority: 112
  - Regex:        '^["<]stan/math/mix/scal'
  - Priority: 113
  - Regex:        '^["<]stan/math/mix/arr'
  - Priority: 114
  - Regex:        '^["<]stan/math/mix/mat'
  - Priority: 115
  - Regex:        '^["<]stan/math/mix/.*'
  - Priority: 116

[syclik]

Yes and no. Yes, what’s written down should be almost up-to-date. The exception is going to be GPU code and the start of the flattening refactor (getting rid of the scal / arr / mat distinction), which came after this. No, this won’t be accurate after we flatten. It’ll be simplified.

…

On Tue, Aug 6, 2019 at 9:41 PM Andrew Ashworth ***@***.***> wrote: Are the comments in this issue still up-to-date? I was considering adding a rule to sort includes in .clang-format via IncludeCategories, ` - Regex: '^["<]stan/math/prim/scal' - Priority: 101 - Regex: '^["<]stan/math/prim/arr' - Priority: 102 - Regex: '^["<]stan/math/prim/mat' - Priority: 103 - Regex: '^["<]stan/math/prim/.*' - Priority: 104 - Regex: '^["<]stan/math/fwd/scal' - Priority: 105 - Regex: '^["<]stan/math/fwd/arr' - Priority: 106 - Regex: '^["<]stan/math/fwd/mat' - Priority: 107 - Regex: '^["<]stan/math/fwd/.*' - Priority: 108 - Regex: '^["<]stan/math/rev/scal' - Priority: 109 - Regex: '^["<]stan/math/rev/arr' - Priority: 110 - Regex: '^["<]stan/math/rev/mat' - Priority: 111 - Regex: '^["<]stan/math/rev/.*' - Priority: 112 - Regex: '^["<]stan/math/mix/scal' - Priority: 113 - Regex: '^["<]stan/math/mix/arr' - Priority: 114 - Regex: '^["<]stan/math/mix/mat' - Priority: 115 - Regex: '^["<]stan/math/mix/.*' - Priority: 116` — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#383?email_source=notifications&email_token=AADH6F3OL2IMWL5OXSJAI7LQDIR4FA5CNFSM4COSAEQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3W52NY#issuecomment-518905143>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADH6FZQBTTZPEN6O7V4GVDQDIR4FANCNFSM4COSAEQA> .