Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support failable initializers #276

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

NiwakaDev
Copy link
Collaborator

Related to #235.

This PR supports failable initializers.

Something like:

// Rust side
#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        #[swift_bridge(Equatable)]
        type FailableInitType;

        #[swift_bridge(init)]
        fn new() -> Option<FailableInitType>;
    }
}
// Swift side
let failableInitType = FailableInitType()
if failableInitType == nil {
    ~
} else {
   ~
}

See: Swift Documentation - Failable Initializers

fn some_function(arg: u8) {}

struct SomeType;

impl SomeType {
#[allow(unused)]
fn some_method(&self, foo: u8) {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added #[allow(unused)] to fix the following warnings.

error: Argument "arg_typo" was not found in "fn some_function(..)"
 --> tests/ui/args-into-argument-not-found.rs:7:42
  |
7 |         #[swift_bridge(args_into = (arg, arg_typo))]
  |                                          ^^^^^^^^

error: Argument "bar" was not found in "fn some_method(..)"
  --> tests/ui/args-into-argument-not-found.rs:13:42
   |
13 |         #[swift_bridge(args_into = (foo, bar))]
   |                                          ^^^

warning: unused variable: `arg`
  --> tests/ui/args-into-argument-not-found.rs:19:18
   |
19 | fn some_function(arg: u8) {}
   |                  ^^^ help: if this is intentional, prefix it with an underscore: `_arg`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `foo`
  --> tests/ui/args-into-argument-not-found.rs:25:27
   |
25 |     fn some_method(&self, foo: u8) {}
   |                           ^^^ help: if this is intentional, prefix it with an underscore: `_foo`

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of #[allow(unused)] let's do _arg and _foo. Less noisy.

@NiwakaDev NiwakaDev requested a review from chinedufn June 11, 2024 16:44
@chinedufn chinedufn changed the title support failable initializers Support failable initializers Jun 13, 2024
Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Left some minor feedback. Thanks for implementing this. Great to see you back.


func testFailableInitializer() {
XCTAssertEqual(FailableInitType(false), nil)
XCTAssertNotEqual(FailableInitType(true), nil)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure that we can use the type. This gives us more confidence that we didn't, say, bridge Some(bool) instead of Some(FailableInitType).

Maybe

let some = FailableInitType(true)
XCTAssertEqual(some!.count(), 123);

@@ -211,5 +211,10 @@ class OptionTests: XCTestCase {
XCTAssertEqual(reflectedSome!.field, 123)
XCTAssertNil(reflectedNone)
}

func testFailableInitializer() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From now on all tests should say what we're verifying.

/// Verify that ...

This helps:

  • new maintainers since it's easier to read English than code.
  • forces the author to be clear about what behavior they're trying to test. Needing to explain ourselves makes us think critically about what we are doing and if we're covering enough cases.
  • the reviewer noticing missing tests. For example, if the reviewer sees that what the test says that it is testing doesn't seem to match the code inside, they can comment on that. This tends to increase the quality of a test suite.

For simple stuff like this a sentence is fine.

Comment on lines 609 to 611
/// Verify that we generated a Swift class with a failable init method.
#[test]
fn class_with_failable_init() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's test this in the codegen_tests module instead of here.

I think we should put all new codegen tests there going forward. This way there is a single place to find all codegen tests.

  • more discoverable
  • less confusion around where to put a test. there's always one place to put a codegen test instead of not knowing if it should go in generate_swift.rs or mod codegen_tests::*

}
extension Foo {
public convenience init?() {
guard let val = __swift_bridge__$Foo$new() else { return nil }; self.init(ptr: val)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@chinedufn
Copy link
Owner

My feedback was minor, so feel free to Squash and merge after you address it.

When you Squash and merge remember to paste in your PR body as the commit message.


Alternatively you can rebase into a single commit, force push that, and then merge that. But Squash and merge essentially does the same thing.

@chinedufn
Copy link
Owner

Hello, hello. Still planning to land this?

@NiwakaDev
Copy link
Collaborator Author

Sorry. I'll address your review this weekend if possible.

@NiwakaDev NiwakaDev force-pushed the feat/failable_init branch 2 times, most recently from b4dade6 to 0125875 Compare August 8, 2024 15:22
@@ -456,3 +456,74 @@ typedef struct MyType MyType;
.test();
}
}

/// Verify that we generated a Swift class with a failable init method.
mod extern_rust_class_with_failable_init {
Copy link
Collaborator Author

@NiwakaDev NiwakaDev Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chinedufn
I guess it's suitable to add the test here.

Related: #276 (comment)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just one small request then you can merge this.

Please remember to squash and merge and use your PR title for the commit title and PR body for the commit body.

@@ -582,6 +582,7 @@
COMBINE_HIDPI_IMAGES = YES;
CURRENT_PROJECT_VERSION = 1;
DEVELOPMENT_ASSET_PATHS = "\"SwiftRustIntegrationTestRunner/Preview Content\"";
DEVELOPMENT_TEAM = "";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these two DEVELOPMENT_TEAM lines? If not lets revert them.

@NiwakaDev NiwakaDev merged commit 495611b into chinedufn:master Aug 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants