-
Notifications
You must be signed in to change notification settings - Fork 92
[swift2objc] Support default parameter values (#1752) #2910
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
base: main
Are you sure you want to change the base?
Conversation
…#1752) - Add defaultValue field to Parameter model to store default expressions - Implement Symbol Graph parser to extract default values from declarationFragments - Generate method/function/initializer overloads for trailing default parameters - Each overload omits trailing defaults, allowing Swift to apply them natively - Add unit tests for parsing single/multiple/string default values - Add integration test fixture demonstrating full default parameter support - All 70 tests passing
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
liamappelbe
left a comment
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.
Make sure you sign the CLA: #2910 (comment)
| import Foundation | ||
|
|
||
| public class Greeter { | ||
| public func greet(name: String = "World", times: Int = 1) -> String { |
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.
Can you test the case of a non-trivial default value? Try an expression like 10 * foo where foo is a static variable on the class. I wonder if something like that would show up as multiple tokens.
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 isn't resolved yet. Add a non-trivial default value to the integration test.
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_function_declaration.dart
Show resolved
Hide resolved
pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart
Outdated
Show resolved
Hide resolved
pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart
Outdated
Show resolved
Hide resolved
|
Make sure to also fix the formatting and analysis errors the bot found. I recommend running |
|
Thanks for the review. I'll work on it. |
…hh/native into fix-default-params-1752 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
I have pushed the final refactor to address all review feedback. The implementation is now much more robust and aligned with the project's architecture: 1-Tokenizer Infrastructure: Moved the token-splitting logic into TokenList.splitToken. It now handles = as a splittable and correctly extracts trailing delimiters from the end of a token. This ensures complex default values are cleanly isolated. All 75 tests are passing locally. |
liamappelbe
left a comment
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.
I get the feeling that a lot of this code is AI generated. That's ok in general, as long as you're checking everything it writes, and making sure you understand it, and think about the implications of the changes you're making. In particular, AI tends to generate a lot of duplicated code, which is the main issue with this PR atm. AI also doesn't do a good job of addressing comments. It's your job to make sure my comments are addressed, and the code is well factorized/merged.
| /// suffix cases. | ||
| /// | ||
| /// Swift's symbol graph concatenates some tokens together, for example when | ||
| /// a parameter has a default value: " = value) -> returnType" becomes a |
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.
Is this true? I don't think the returnType can be part of the token. If it can, then just taking splittables of the start and end won't work.
|
|
||
| if (foundPrefix) continue; | ||
|
|
||
| // No more prefix splittables found; extract any trailing ones. |
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.
Move this out into its own while loop below the prefix extractor. Makes it more readable.
| } | ||
|
|
||
| var suffix = text; | ||
| var remaining = text; |
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.
I think this rename makes this more confusing. You've named this variable remaining, and the other one currentSuffix, but that's kinda the opposite of what they actually represent. We first remove prefixes, so what's left is a suffix. Then we remove suffixes, so what's left isn't really a suffix, and could be better named something generic like remaining. So I'd probably switch these names.
| var afterType = maybeConsume('text'); | ||
|
|
||
| if (afterType != null) { | ||
| // Check if this text token contains '=' |
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.
A lot of this logic is outdated, now that all the token splitting is done in the token_list. If the token list is working correctly, you shouldn't ever need to further split tokens here, or do any string parsing at all. Each of the parts that you're checking for with these .startsWith and .contains calls should now be a separate token in the list that you receive from token_list.dart, and the default value itself should also be a single token.
It's often hard to modify complex code like this after such a big logic change elsewhere in the system, so I'd approach this by deleting this whole chunk of code, and starting over. Print out the token list that is received here, make sure it makes sense (ie, all tokens are split as you expect, and the default value is its own token), and then write a new parser for that token list. If you find yourself having to use String.split/substring/startsWith/contains in this area, then something is probably wrong in the token list logic.
If things work out like I'm expecting, then this whole chunk of code will be waaaay simpler after that rewrite.
| ), | ||
| ) | ||
| .toList(); | ||
| final transformedInitializers = <Declaration>[]; |
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.
Dart's fancy new collection literals can simplify this a bit. Something like this should work:
final transformedInitializers = <Declaration>[
for (final init in originalCompound.initializers)
...transformInitializerWithOverloads(
init,
wrappedCompoundInstance,
parentNamer,
state,
),
];Same below
| ); | ||
|
|
||
| return transformedMethod; | ||
| // Generate overloads for trailing default parameters. Each overload omits |
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.
You haven't actually deduped the logic here, just combined the functions into one. Try to merge everything below here in this function with everything above. Eg, what happens if you let parametersToOmit start its loop at 0 instead of 1?
| return (instanceConstruction, localNamer); | ||
| } | ||
|
|
||
| int _trailingDefaultCount(List<Parameter> params) { |
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.
dupe of the one in transform_compound
| return count; | ||
| } | ||
|
|
||
| List<String> _generateInitializerStatementsWithSubset( |
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.
can be merged with _generateInitializerStatements
| } | ||
| } | ||
|
|
||
| (String, UniqueNamer) _generateInstanceConstructionWithSubset( |
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.
Can be merged with _generateInstanceConstruction
| import Foundation | ||
|
|
||
| public class Greeter { | ||
| public func greet(name: String = "World", times: Int = 1) -> String { |
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 isn't resolved yet. Add a non-trivial default value to the integration test.
Hi Liam sir, thanks for being direct. I’m sorry for the oversight on the code quality and the duplication; I relied too much on AI to scaffold the serialization logic and didn't spend enough time factorizing it to meet the project's standards. I have taken your feedback to heart. I'm going to step back and manually refactor the PR to remove the redundant logic and ensure all your comments are addressed individually. I want to make sure the final result is something I fully understand and can maintain. I’ll ping you when the next push is ready for a fresh look. |
Description
This PR implements support for default parameter values in swift2objc by generating Objective-C compatible method overloads. Since Objective-C does not natively support default parameters, the wrapper now provides multiple versions of the same function, allowing the consumer to omit trailing arguments while preserving the original Swift default behavior.
Technical Changes$N$ overloads. This implementation supports methods, global functions, and initializers.
1- Model Layer: Added a defaultValue field to the Parameter model to store the default expression as a string.
2 - Parser Layer: Implemented logic in the Symbol Graph parser to extract default values from declarationFragments, handling both single and multi-token expressions.
3 - Transformer Layer: Updated the transformation logic to identify trailing parameters with defaults and generate
4 - Initializer Special Case: Resolved a mismatch in the expected output regarding @objc override public init() overloads to ensure proper Objective-C interop for default initializers.
Testing and Verification
1- Added unit tests to verify the correct parsing of numeric, string, and complex expression default values from the Symbol Graph.
2- Introduced an integration test fixture (default_params_input.swift) to demonstrate the end-to-end generation of overloaded wrapper code.
3- Verified the implementation against the full suite, with all 70 integration tests currently passing.
Open for your suggestion @liamappelbe :)
Fixes #1752