Skip to content

[PropertyWrappers] Look for constructors with default args when finding default init #27253

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

Merged
merged 8 commits into from
Oct 12, 2019
Merged

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Sep 19, 2019

At the moment, findDefaultInit() only looks for init() and ignores any constructors that have default args. This is a bug and it prevents the following code from working:

@propertyWrapper
struct Foo {
  let name: String

  init(name: String = "hello world") {
    self.name = name
  }

  var wrappedValue: Int {
    get { return 0 }
  }
}

struct Bar {
  @Foo var property: Int // error: property not initialized
  init() {}
}

When finding default initializers, also search for constructors with default args.

Resolves SR-11477.

@theblixguy theblixguy changed the title [PropertyWrappers] Look for constructors with default values when finding default init [PropertyWrappers] Look for constructors with default args when finding default init Sep 19, 2019
@theblixguy
Copy link
Collaborator Author

theblixguy commented Sep 20, 2019

I also noticed we didn't have any tests for multiple default inits, so I have now added some typechecker tests as well. I am going to run a quick test now while I wait for @DougGregor's review. Funny enough, the only way to trigger that diagnostic was to have two init() which would've triggered a redeclaration diagnostic anyway 😄

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please test source compatibility

@theblixguy
Copy link
Collaborator Author

@DougGregor Do you have any further feedback on this?

@DougGregor
Copy link
Member

Sorry, I caused a conflict when I merged #27322

@theblixguy
Copy link
Collaborator Author

@DougGregor No problem, I have rebased now 👍 Can you take another look at this?

@theblixguy
Copy link
Collaborator Author

@DougGregor I have combined both methods into one as per your suggestion. Does it look good to you?

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is greatly improved! A few minor comments about the control flow in the unified lookup, but this will be ready to go in. Thank you!

@theblixguy
Copy link
Collaborator Author

@DougGregor Thanks for the review! I have addressed your comments :)

@theblixguy
Copy link
Collaborator Author

ping @DougGregor, can you approve this if you're happy? thank you!

@theblixguy
Copy link
Collaborator Author

A conflict creeped in - fixed it! @DougGregor just waiting for your approval :-)

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Thank you! Looks fantastic.

@DougGregor
Copy link
Member

@swift-ci please smoke test

1 similar comment
@DougGregor
Copy link
Member

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please test source compatibility

@theblixguy
Copy link
Collaborator Author

Debug source compat failure is an unrelated UPASS.

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.

4 participants