-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Conversation
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 @swift-ci please smoke test |
@swift-ci please smoke test |
@swift-ci please test source compatibility |
@DougGregor Do you have any further feedback on this? |
Sorry, I caused a conflict when I merged #27322 |
@DougGregor No problem, I have rebased now 👍 Can you take another look at this? |
@DougGregor I have combined both methods into one as per your suggestion. Does it look good to you? |
There was a problem hiding this 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!
@DougGregor Thanks for the review! I have addressed your comments :) |
ping @DougGregor, can you approve this if you're happy? thank you! |
A conflict creeped in - fixed it! @DougGregor just waiting for your approval :-) |
There was a problem hiding this 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.
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
…und by findDefaultInit()
…e extra 'hasParams' check
…we have multiple inits, before diagnosing them as ambiguous
… methods into a single 'findSuitableWrapperInit' method
…fixing merge conflict
@swift-ci please smoke test |
@swift-ci please test source compatibility |
Debug source compat failure is an unrelated UPASS. |
At the moment,
findDefaultInit()
only looks forinit()
and ignores any constructors that have default args. This is a bug and it prevents the following code from working:When finding default initializers, also search for constructors with default args.
Resolves SR-11477.