cc @reitermarkus @pvdrz

I commented on #2543 with some stuff that I found at a glance but I don't think it necessarily causes this bug...

@glandium I also needed -std=c++17 for the test-case to work fwiw, maybe you can update the OP?

I'm fairly confused at #2543. It only sorts the direct children of a cursor, how does it deal with C++ namespaces or other cursors with nested children for which we return ParseError::Recurse? Do those not need to be sorted?

Another probably more relevant data point. If I remove the -include flags and change them by actual #include directives in the first file, then it works. That probably explains why it doesn't happen in the preprocessed source.

We're probably relying on seeing a declaration before a template instantiation and somehow bindgen is getting the sorting wrong because of the -include flag.

Another probably more relevant data point. If I remove the -include flags and change them by actual #include directives in the first file, then it works. That probably explains why it doesn't happen in the preprocessed source.

See one of the many ways to make it work mentioned in OP ;)
That said, I think the reason it doesn't happen in the preprocessed source is different, because in that case, the locations are all associated with that one file.

BTW, I should mention that replacing the visit_sorted in parse with visit makes it work too (without touching parse_one or ClangSubItemParser)

Here's an actually reduced test-case. Still multiple headers because of the nature of the issue: reduced.tar.gz

To reproduce:

$ ./target/debug/bindgen --no-layout-tests reduced/GeckoBindings.h --enable-cxx-namespaces --generate types -- -x c++ -std=c++17 -include reduced/nsStyleStruct.h -include reduced/LayoutConstants.h

The nsSize struct ends up in the foo namespace, rather than in the root.

Further reduced: reduced.tar.gz

$ ./target/debug/bindgen --no-layout-tests reduced/GeckoBindings.h -- -x c++ -include reduced/nsStyleStruct.h -include reduced/LayoutConstants.h

Actual output:

/* automatically generated by rust-bindgen 0.66.0 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct foo_nsSize {
    pub width: ::std::os::raw::c_int,
    pub height: ::std::os::raw::c_int,
}
extern "C" {
    #[link_name = "\u{1}_ZN3fooL22kFallbackIntrinsicSizeE"]
    pub static foo_kFallbackIntrinsicSize: foo_nsSize;
}

Expected output:

/* automatically generated by rust-bindgen 0.66.0 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct nsSize {
    pub width: ::std::os::raw::c_int,
    pub height: ::std::os::raw::c_int,
}
extern "C" {
    #[link_name = "\u{1}_ZN3fooL22kFallbackIntrinsicSizeE"]
    pub static foo_kFallbackIntrinsicSize: nsSize;
}

Not a surprise of course, but the sorting is going rather wrong:

before sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize")),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo"))
]
after sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo")),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/nsStyleStruct.h:3:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize"))
]

I don't think the source order comparison is working right to begin with, it only deals with one level of includes?

In the case of LayoutConstants.h, for example, which is not included by anything, anything in it would be considered first than anything in nsSize.h, even though nsSize.h is included by LayoutConstants.h!

The other thing I don't understand is why do we need to do this sorting at all? The original sorting from clang looks right.

The other thing I don't understand is why do we need to do this sorting at all?

clang groups all items by type. So e.g. all macros are sorted correctly relative to each other, but all macros are sorted before e.g. all variables.

Now if you have this

#define VAR 1
#undef VAR
const int VAR = 2;

it would generate VAR 1 with clang sorting. By sorting based on source order, we can reason that VAR was undefined since we see a constant with the same name later, and correctly generate VAR 2.

@reitermarkus why do we need the var to be sorted correctly relatively to the macro? The var is already correctly macro-expanded etc, that's the only reason bindgen can deal with token pasting etc. The macro information that clang provides should be treated probably rather separately to everything else. And that still would work for a test-case like the one you pasted, right?

In any case the source order sorting as implemented today is clearly incorrect. I ran out of time to fix it for the day, so I sent a PR to disable temporarily, see #2558.

I think that's the best course of action for now until it can be adapted to deal with multiple layers of includes properly.

The macro information that clang provides should be treated probably rather separately to everything else.

In #2369, it is treated separately. The issue is, macros are parsed first, so in codegen macros are also generated first. And we generate constants for variable-like macros as well as for real constants.

Ok, seems those conflicts should be dealt with at codegen time? Or am I missing something? Or, rather, there's no reason we couldn't run codegen for macro-generated constants later, to "favor" real constants

to "favor" real constants

Then this

const int VAR = 1;
#define VAR 2

would wrongly generate VAR 1. Granted this is very much an edge case.

Not a surprise of course, but the sorting is going rather wrong:

before sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize")),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo"))
]
after sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo")),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/nsStyleStruct.h:3:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize"))
]

I don't think the source order comparison is working right to begin with, it only deals with one level of includes?

In the case of LayoutConstants.h, for example, which is not included by anything, anything in it would be considered first than anything in nsSize.h, even though nsSize.h is included by LayoutConstants.h!

I noticed this in 83f729f but decided to ignore it for some reason 🤔. This is starting to sound like you need to topologically sort the cursors based on the partial order defined there instead of just sorting them in a binary way. In that way any cursor that appears before another in the list is guaranteed to not depend on the latter.

Can you spin a 0.66.1 release with the fix?

(there are failing tests on CI, though)

Ugh I think that's because the clang 5 and 9 tests needed to be reverted too. I'll take a look.

The PR for 0.66.1 has already been open #2559