fix: resolve path types for 'both' style

[Wodann]

Potentially fixes #215.

Types of structs were not being resolved when the style was set to 'both', resulting in the struct keyword not being added to struct field types. This caused compiler errors complaining about missing forward declarations.

[Wodann]

I couldn't test these changes locally because cargo test fails with the following error - even on latest:

test bin_explicit_release_build ... ok
test lib_explicit_release_build ... FAILED
test bin_default_uses_debug_build ... ok
test bin_explicit_debug_build ... ok
test lib_explicit_debug_build ... FAILED
test lib_default_uses_debug_build ... FAILED

---- lib_explicit_release_build stdout ----
thread 'lib_explicit_release_build' panicked at 'build should succeed: CargoExpand("expand-dep", Compile("   Compiling dep v0.1.0 (E:\\dev\\src\\rust\\cbindgen\\tests\\rust\\expand_dep\\dep)\n     Running `rustc --crate-name dep --edition=2018 dep\\src\\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C metadata=ac7708978d9e5b8b -C extra-filename=-ac7708978d9e5b8b --out-dir C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps -L dependency=C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps`\n   Compiling expand-dep v0.1.0 (E:\\dev\\src\\rust\\cbindgen\\tests\\rust\\expand_dep)\n     Running `rustc --crate-name expand_dep --edition=2018 src\\lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -Z unstable-options --pretty=expanded -C metadata=46e5aad5969eaffb -C extra-filename=-46e5aad5969eaffb --out-dir C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps -L dependency=C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps --extern dep=C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps\\libdep-ac7708978d9e5b8b.rmeta`\nerror: the option `Z` is only accepted on the nightly compiler\n\nerror: could not compile `expand-dep`.\n\nCaused by:\n  process didn\'t exit successfully: `rustc --crate-name expand_dep --edition=2018 src\\lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -Z unstable-options --pretty=expanded -C metadata=46e5aad5969eaffb -C extra-filename=-46e5aad5969eaffb --out-dir C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps -L dependency=C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps --extern dep=C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-output-RHdj9Z\\release\\deps\\libdep-ac7708978d9e5b8b.rmeta` (exit code: 1)\nwarning: build failed, waiting for other jobs to finish...\nerror: build failed\n"))', tests\profile.rs:25:24
stack backtrace:
   0:     0x7ff6395d8ab9 - std::backtrace_rs::backtrace::dbghelp::trace
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\..\..\backtrace\src\backtrace\dbghelp.rs:98
   1:     0x7ff6395d8ab9 - std::backtrace_rs::backtrace::trace_unsynchronized
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\..\..\backtrace\src\backtrace\mod.rs:66
   2:     0x7ff6395d8ab9 - std::sys_common::backtrace::_print_fmt
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys_common\backtrace.rs:79
   3:     0x7ff6395d8ab9 - std::sys_common::backtrace::_print::{{impl}}::fmt
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys_common\backtrace.rs:58
   4:     0x7ff6395f7cdb - core::fmt::write
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\core\src\fmt\mod.rs:1082
   5:     0x7ff6390f1d9c - std::io::Write::write_fmt<test::helpers::sink::Sink>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\io\mod.rs:1514
   6:     0x7ff6395d1248 - std::io::impls::{{impl}}::write_fmt<Write>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\io\impls.rs:176
   7:     0x7ff6395dc2fb - std::sys_common::backtrace::_print
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys_common\backtrace.rs:61
   8:     0x7ff6395dc2fb - std::sys_common::backtrace::print
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys_common\backtrace.rs:48
   9:     0x7ff6395dc2fb - std::panicking::default_hook::{{closure}}
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\panicking.rs:200
  10:     0x7ff6395dbed8 - std::panicking::default_hook
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\panicking.rs:216
  11:     0x7ff6395dcadf - std::panicking::rust_panic_with_hook
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\panicking.rs:569
  12:     0x7ff6395dc645 - std::panicking::begin_panic_handler::{{closure}}
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\panicking.rs:476
  13:     0x7ff6395d936f - std::sys_common::backtrace::__rust_end_short_backtrace<closure-0,!>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys_common\backtrace.rs:153
  14:     0x7ff6395dc5f9 - std::panicking::begin_panic_handler
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\panicking.rs:475
  15:     0x7ff6395f5a40 - core::panicking::panic_fmt
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\core\src\panicking.rs:85
  16:     0x7ff6395f5863 - core::option::expect_none_failed
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\core\src\option.rs:1221
  17:     0x7ff63907bb33 - core::result::Result<cbindgen::bindgen::bindings::Bindings, cbindgen::bindgen::error::Error>::expect<cbindgen::bindgen::bindings::Bindings,cbindgen::bindgen::error::Error>
                               at C:\Users\Remco\.rustup\toolchains\1.47.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\result.rs:933
  18:     0x7ff63907851a - profile::build_using_lib
                               at E:\dev\src\rust\cbindgen\tests\profile.rs:25
  19:     0x7ff6390e9139 - profile::lib_explicit_release_build::{{closure}}
                               at E:\dev\src\rust\cbindgen\tests\profile.rs:79
  20:     0x7ff6390456ab - core::ops::function::FnOnce::call_once<closure-0,tuple<>>
                               at C:\Users\Remco\.rustup\toolchains\1.47.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227      
  21:     0x7ff63913e94e - serial_test::serial_core
                               at C:\Users\Remco\.cargo\registry\src\github.com-1ecc6299db9ec823\serial_test-0.5.0\src\lib.rs:64
  22:     0x7ff639078a5b - profile::lib_explicit_release_build
                               at E:\dev\src\rust\cbindgen\tests\profile.rs:77
  23:     0x7ff6390e907e - profile::lib_explicit_release_build::{{closure}}
                               at E:\dev\src\rust\cbindgen\tests\profile.rs:78
  24:     0x7ff63904584b - core::ops::function::FnOnce::call_once<closure-0,tuple<>>
                               at C:\Users\Remco\.rustup\toolchains\1.47.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227      
  25:     0x7ff6391236f6 - core::ops::function::FnOnce::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\core\src\ops\function.rs:227
  26:     0x7ff6391236f6 - test::__rust_begin_short_backtrace<fn()>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\test\src\lib.rs:517
  27:     0x7ff639121041 - alloc::boxed::{{impl}}::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\alloc\src\boxed.rs:1042
  28:     0x7ff639121041 - std::panic::{{impl}}::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panic.rs:308
  29:     0x7ff639121041 - std::panicking::try::do_call
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panicking.rs:373
  30:     0x7ff639121041 - std::panicking::try
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panicking.rs:337
  31:     0x7ff639121041 - std::panic::catch_unwind
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panic.rs:379
  32:     0x7ff639121041 - test::run_test_in_process
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\test\src\lib.rs:544
  33:     0x7ff639121041 - test::run_test::run_test_inner::{{closure}}
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\test\src\lib.rs:450
  34:     0x7ff6390f10e6 - std::sys_common::backtrace::__rust_begin_short_backtrace<closure-0,tuple<>>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\sys_common\backtrace.rs:137
  35:     0x7ff6390f10e6 - std::sys_common::backtrace::__rust_begin_short_backtrace<closure-0,tuple<>>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\sys_common\backtrace.rs:137
  36:     0x7ff6390f7f7e - std::thread::{{impl}}::spawn_unchecked::{{closure}}::{{closure}}
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\thread\mod.rs:458
  37:     0x7ff6390f7f7e - std::panic::{{impl}}::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panic.rs:308
  38:     0x7ff6390f7f7e - std::panicking::try::do_call
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panicking.rs:373
  39:     0x7ff6390f7f7e - std::panicking::try
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panicking.rs:337
  40:     0x7ff6390f7f7e - std::panic::catch_unwind
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\panic.rs:379
  41:     0x7ff6390f7f7e - std::thread::{{impl}}::spawn_unchecked::{{closure}}
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\std\src\thread\mod.rs:457
  42:     0x7ff6390f7f7e - core::ops::function::FnOnce::call_once<closure-0,tuple<>>
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\core\src\ops\function.rs:227
  43:     0x7ff6395e682a - alloc::boxed::{{impl}}::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\alloc\src\boxed.rs:1042
  44:     0x7ff6395e682a - alloc::boxed::{{impl}}::call_once
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\library\alloc\src\boxed.rs:1042
  45:     0x7ff6395e682a - std::sys::windows::thread::{{impl}}::new::thread_start
                               at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39\/library\std\src\sys\windows\thread.rs:56
  46:     0x7ff9804e7034 - BaseThreadInitThunk
  47:     0x7ff9812fcec1 - RtlUserThreadStart

[Wodann]

@eqrion @emilio is there an easy way of updating the expectations to include struct in struct field types for the 'both' style? Or should I manually update all of the .both files?

[emilio]

Running cargo +nightly test should automatically update them. It might be that the tests introduced in #615 don't work on Windows. If you comment them out do the rest of the tests pass?

[emilio]

Err, in #607 I mean.

[emilio]

Here's a patch which updates the existing tests fwiw, though we also need a test for what this fixes... 0001-Test-updates.txt

[emilio]

The fix looks sensible though, thanks for digging into it!

[Wodann]

Running cargo +nightly test should automatically update them. It might be that the tests introduced in #615 don't work on Windows. If you comment them out do the rest of the tests pass?

Thanks; I am running on Windows. Those initial tests pass when I use that command but all other test fail due invalid paths:

---- test_workspace stdout ----
Running: "E:\\dev\\src\\rust\\cbindgen\\target\\debug\\cbindgen" "--lang" "c" "--cpp-compat" "--style" "type" "E:\\dev\\src\\rust\\cbindgen\\tests\\rust\\workspace"
Running: "gcc" "-D" "DEFINED" "-I" "E:\\dev\\src\\rust\\cbindgen\\tests" "-Wall" "-Werror" "-Wno-attributes" "-Wno-unused-const-variable" "-Wno-return-type-c-linkage" "-D" "CBINDGEN_STYLE_TYPE" "-o" "C:\\Users\\Remco\\AppData\\Local\\Temp\\cbindgen-test-outputQDc2OQ\\workspace.compat.o" "-c" "E:\\dev\\src\\rust\\cbindgen\\tests\\expectations\\workspace.compat.c"
thread 'test_workspace' panicked at 'failed to compile: Os { code: 2, kind: NotFound, message: "The system cannot find the file specified." }', tests\tests.rs:121:32

Here's a patch which updates the existing tests fwiw, though we also need a test for what this fixes... 0001-Test-updates.txt

Thanks for the patch. That makes life a lot easier 🙂

I'll add a test with an incomplete type. Currently it will never work for the tag style though. Can I exclude that test-case, or is that an issue?

[emilio]

Thanks; I am running on Windows. Those initial tests pass when I use that command but all other test fail due invalid paths:

It seems you don't have gcc in your path, if you have clang or another compiler in the path you could do CC=clang CXX=clang++ cargo +nightly test or such, for example.

[Wodann]

Thanks. I fixed my WSL2 and was able to get cargo +nightly test to work.

It seems that some expectations were missing; even before my changes. I added those, and will make sure to add an additional test for forward declarations tomorrow.

[Wodann]

I've added a forward_declaration.rs test, but it will always fail for the Style::Type and C++ test (which defaults to Style::Type it seems).

I don't think the Style::Type can ever work with forward declarations, because there is no struct name. How would you recommend I proceed? Should I add functionality for specifying which styles to run a test for?

Also, it seems that the style option is broken; i.e. it does not actually default to Both, even though the docs say so:

# Codegen Options

# When generating a C header, the kind of declaration style to use for structs
# or enums.
#
# possible values:
# * "type": typedef struct { ... } MyType;
# * "tag": struct MyType { ... };
# * "both": typedef struct MyType { ... } MyType;
#
# default: "both"
style = "both"

[Wodann]

I tried several things to avoid the test for Style::Type to fail, but this was the only way to actually get it to work. Forward declaration just cannot work for that style, and this allows me to completely skip the compilation test - thus succeeding.

[Wodann]

I've added a test and got the build to succeed.

If there's anything else I can improve, please let me know.

[Wodann]

@emilio should I split the PR into multiple ones?

[emilio]

No, it's fine, I just need to get to reviewing these, sorry for the lag (been swamped with other stuff lately)

[emilio]

This looks good. I took the liberty of rebasing for you (but kept you as the author of course).

Thanks for the fix!

[petrochenkov]

I've noticed that cbindgen adds a lot of struct/union noise on master compared to previous versions, and the search led me to this PR.

@Wodann

Types of structs were not being resolved when the style was set to 'both', resulting in the struct keyword not being added to struct field types.

This was by design, as I understand.
Why do you need structs if the structures are defined in "both" style as typedef struct Foo { ... } Foo;?

Looks like the issue in #215 (comment) was with incorrect forward declarations rather than with missing structs?
Although it's not clear since the code snippet is incomplete.

[petrochenkov]

This looks like a regression to me.

[emilio]

The main issue this fixes is something like recursive structs where you have:

typedef struct Foo {
  Foo* bar;
} Foo;

[emilio]

(Where you do need the struct in the member)

[emilio]

So yeah, while I agree it's more noisy, it's more correct too.

[emilio]

So we could reduce the noise level a bit by resolving or outputting ctypes only when necessary, and should be relatively straight-forward...

@petrochenkov would you be fine with outputting ctypes for styles that generate typedefs only on members, or something of that sort? That'd be the easiest check, we could potentially go smarter and try to detect the self-referencing case, but not sure to what extent it's worth it.

[petrochenkov]

I see, the recursive case is a legitimate one.
I'll look at this in more detail in couple of days.

[petrochenkov]

One more thing - the forward_declaration test doesn't actually work.
Block comments /* ... */ have higher "priority" here than preprocessor directives, so

#if defined(CBINDGEN_STYLE_TYPE)
/* ANONYMOUS STRUCTS DO NOT SUPPORT FORWARD DECLARATIONS!
#endif

...

#if defined(CBINDGEN_STYLE_TYPE)
*/
#endif

is equivalent to just

#if defined(CBINDGEN_STYLE_TYPE)
/* ... */
#endif

[petrochenkov]

Ok, this is not trivial to fix, so I'll leave this for later (if I get to #592 I'll try to fix this as well).

What I think is a proper solution: when we do the topological sort of items to satisfy dependencies and detect cycles

• we insert forward declaration nodes (typedef struct Foo Foo;) until there are no cycles
• we add tag to items involved in the forward declarations even in Style::Type mode, so the generated code compiles in all modes.

[Wodann]

@petrochenkov sorry for being late to the party. Good catch with the preprocessor order. Did you already fix that issue? It seems like some changes were made to the particular *.toml file.

[petrochenkov]

@Wodann
No, I didn't do anything yet, the changes were done during merge to fix conflicts with #620.