Types can end up in the wrong namespace

[glandium]

This is a regression from 83f729f

Input C/C++ Header

I'm sorry this is not reduced as much as possible, but it's already significantly reduced. The problem does not happen using the output of --dump-preprocessed-input (it does require multiple separate headers, and even worse, it requires that some of them are included multiple times)

The complete set of files involved in this somehow reduced testcase is: https://drive.google.com/file/d/1vldgLGgcslyeUTlcHzMd4r0WVZNY2Wht/view?usp=sharing

Bindgen Invocation

Assuming the testcase.tar.gz archive is extracted to /tmp/testcase, run the cli with

cargo run --release -p bindgen-cli -- /tmp/testcase/mozilla/GeckoBindings.h --allowlist-type nsSimpleContentList --allowlist-type nsSize --enable-cxx-namespaces --no-derive-default --generate types --ignore-methods --rust-target 1.68 -- -x c++ -std=c++17 -fno-sized-deallocation -fno-aligned-new -DTRACING=1 -DIMPL_LIBXUL -DMOZILLA_INTERNAL_API -DRUST_BINDGEN -I/tmp/testcase -I/tmp/testcase/nspr -include /tmp/testcase/mozilla-config.h -include /tmp/testcase/nsStyleStruct.h -include /tmp/testcase/LayoutConstants.h

(using clang 16, I don't know for sure whether the command works with older versions, I know for sure it doesn't work with clang 11, because I have that in a corner)

Actual Results

The output contains the nsSize type in the root::mozilla namespace, although the type is defined in the root namespace.

There are many ways to make this disappear:

• removing --allowlist-type nsSimpleContentList
• in LayoutConstants.h, adding constexpr nsSize dummy(0, 0); above namespace mozilla (because yes, that's where the bogus namespace comes from)
• not including nsStyleStruct.h from the command line
• not including nsStyleStruct.h from mozilla/GeckoBindings.h
• using the preprocessed input
• creating a file that includes mozilla-config.h, nsStyleStruct.h, LayoutConstants.h and mozilla/GeckoBindings.h, and using that instead of passing those files on the command line.
• etc.

[emilio]

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...

[emilio]

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

[emilio]

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?

[emilio]

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.

[emilio]

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.

[glandium]

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.

[glandium]

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

[emilio]

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.

[emilio]

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;
}

[emilio]

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!

[emilio]

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.

[reitermarkus]

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.