Fix non-deterministic generation order #230

NiwakaDev · 2023-06-17T06:28:01Z

Related to #225.

Before this commit, the integration tests sometimes fail, like so:

field has incomplete type 'struct swift_bridge$tuple$I32ResultTestOpaqueRustTypeString'

This PR fixes the issue.

NiwakaDev · 2023-06-17T06:32:32Z

It seems like another issue happens:

Test session results, code coverage, and logs:
	Command CompileSwift failed with a nonzero exit code
	/Users/runner/Library/Developer/Xcode/DerivedData/SwiftRustIntegrationTestRunner-eunjzlpbulmkoedlbxmntxkdbksm/Logs/Test/Run-SwiftRustIntegrationTestRunner-2023.06.17_06-29-48-+0000.xcresult
	Cannot find 'swift_reflect_already_declared_struct' in scope
	Testing cancelled because the build failed.

NiwakaDev · 2023-06-17T07:13:25Z

I rebased #226 into this branch, and then I fixed the immediately above issue.

NiwakaDev · 2023-06-17T07:16:00Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/SharedStructAttributes.swift

+func swift_reflect_already_declared_struct(arg: AlreadyDeclaredStructTest) -> AlreadyDeclaredStructTest {
+    arg
+}
+


To fix the immediately above issue, I appended this function.

chinedufn

AWESOME! Left some minor feedback but after that LGTM. Thanks so much for solving this problem. Solution looks good.

chinedufn · 2023-06-19T19:36:19Z

crates/swift-bridge-ir/src/codegen/codegen_tests/c_ffi_declaration_order_codegen_tests.rs

+use quote::quote;
+
+/// Verify that fields of a structure in a C header file are generated before the structure.
+mod structure_fields_generated_before_structure {


Maybe

/// Verify that the type that if there is a `Result<Tuple, _>` the generated C header contains /// the tuple's fields followed by the tuple's FFI representation followed by the Result FFI repr. mod tuple_fields_generated_before_tuple_generated_before_result

chinedufn · 2023-06-19T19:37:50Z

crates/swift-bridge-ir/src/codegen/codegen_tests/c_ffi_declaration_order_codegen_tests.rs

+                extern "Rust" {
+                    fn rust_func_return_result_tuple_transparent_enum(
+                        succeed: bool,
+                    ) -> Result<(i32, ResultTestOpaqueRustType, String), i32>;


Maybe Result<(RustOpaqueType1, RustOpaqueType1), (RustOpaqueType2, RustOpaqueType2)> ?
(I can't remember if we allow tuples in the E of Result<T, E>)

I don't implement it, but it might work.

Here's a PR which adds support Result<tuple, _>
#213

chinedufn · 2023-06-19T19:41:16Z

crates/swift-bridge-ir/src/codegen/generate_c_header.rs

@@ -16,6 +16,11 @@ struct Bookkeeping {
    slice_types: HashSet<String>,
 }

+struct CFFiStructDeclarationBookkeeping {


I think that most of the documentation that you put on CFFiStruct belongs here instead.

As in, we should document why we do this bookkeeping.

chinedufn · 2023-06-19T19:43:06Z

crates/swift-bridge-ir/src/bridged_type.rs

+/// For example, struct FooStruct {struct BarStruct bar_struct;} is generated in the order of
+/// struct BarStruct {int32_t value;};
+/// struct struct FooStruct {struct BarStruct bar_struct;};
+///
+/// Please see https://github.com/chinedufn/swift-bridge/pull/225.


I think that we can move this example to CFFiStructDeclarationBookkeeping

The docs on CFFiStructDeclarationBookkeeping should

Explain the problem

Explain how we use the struct to bookkeeping struct solve the problem

I think we should delete the link to the pull request. Anything relevant should be explained in the code doc.

chinedufn · 2023-06-19T19:43:36Z

crates/swift-bridge-ir/src/bridged_type.rs

+/// struct struct FooStruct {struct BarStruct bar_struct;};
+///
+/// Please see https://github.com/chinedufn/swift-bridge/pull/225.
+pub(crate) struct CFFiStruct {


CFfiStruct

chinedufn · 2023-06-19T19:44:04Z

crates/swift-bridge-ir/src/codegen/codegen_tests/c_ffi_declaration_order_codegen_tests.rs

@@ -0,0 +1,55 @@
+use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode};


Let's add the work header to this file. Maybe c_header_declaration_order_codegen_tests

You mean:

Replace c_ffi_declaration_order_generation_tests with c_header_declaration_order_codegen_tests.rs.

?

yes sorry i meant rename. was a typo

chinedufn · 2023-06-19T19:45:02Z

crates/swift-bridge-ir/src/codegen/generate_c_header.rs

@@ -16,6 +16,11 @@ struct Bookkeeping {
    slice_types: HashSet<String>,
 }

+struct CFFiStructDeclarationBookkeeping {


CFfiStructDeclarationBookkeeping

NiwakaDev · 2023-06-20T03:44:28Z

@chinedufn
I guess that I addressed your review. But, If we need to support Result<tuple, tuple>, we couldn't yet merge this PR into master.

NiwakaDev force-pushed the fix_test_faling branch from edae3d8 to 3db7ced Compare June 17, 2023 06:29

NiwakaDev force-pushed the fix_test_faling branch from 3db7ced to 100340c Compare June 17, 2023 06:35

Fix the non-deterministic generation order

8786ed2

NiwakaDev force-pushed the fix_test_faling branch from 100340c to 8786ed2 Compare June 17, 2023 07:05

NiwakaDev requested a review from chinedufn June 17, 2023 07:13

NiwakaDev commented Jun 17, 2023

View reviewed changes

NiwakaDev changed the title ~~Fix the non-deterministic generation order~~ Fix non-deterministic generation order Jun 19, 2023

chinedufn reviewed Jun 19, 2023

View reviewed changes

Address review

0056d55

NiwakaDev force-pushed the fix_test_faling branch from b9b2543 to 0056d55 Compare June 20, 2023 03:38

chinedufn mentioned this pull request Jun 20, 2023

Swift Vec<primitive> Return Type and Function Argument #229

Merged

chinedufn merged commit 4fbd30f into chinedufn:master Jun 20, 2023

chinedufn mentioned this pull request Jun 29, 2023

Make nested struct declarations order independent #170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix non-deterministic generation order #230

Fix non-deterministic generation order #230

NiwakaDev commented Jun 17, 2023 •

edited

Loading

NiwakaDev commented Jun 17, 2023 •

edited

Loading

NiwakaDev commented Jun 17, 2023

NiwakaDev Jun 17, 2023 •

edited

Loading

chinedufn left a comment

chinedufn Jun 19, 2023

chinedufn Jun 19, 2023

NiwakaDev Jun 19, 2023

chinedufn Jun 19, 2023

chinedufn Jun 19, 2023

chinedufn Jun 19, 2023

chinedufn Jun 19, 2023

NiwakaDev Jun 19, 2023

chinedufn Jun 20, 2023

chinedufn Jun 19, 2023

NiwakaDev commented Jun 20, 2023 •

edited

Loading

		@@ -0,0 +1,55 @@
		use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode};

Fix non-deterministic generation order #230

Fix non-deterministic generation order #230

Conversation

NiwakaDev commented Jun 17, 2023 • edited Loading

NiwakaDev commented Jun 17, 2023 • edited Loading

NiwakaDev commented Jun 17, 2023

NiwakaDev Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiwakaDev commented Jun 20, 2023 • edited Loading

NiwakaDev commented Jun 17, 2023 •

edited

Loading

NiwakaDev commented Jun 17, 2023 •

edited

Loading

NiwakaDev Jun 17, 2023 •

edited

Loading

NiwakaDev commented Jun 20, 2023 •

edited

Loading