-
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?
Changes from all commits
62f8795
0e087b9
97b8966
fa5929b
2e63fed
2623056
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,42 +30,79 @@ class TokenList extends Iterable<Json> { | |
| return TokenList._(list, 0, list.length); | ||
| } | ||
|
|
||
| /// Splits a single token on splittable characters, handling both prefix and | ||
| /// suffix cases. | ||
| /// | ||
| /// Swift's symbol graph concatenates some tokens together, for example when | ||
| /// a parameter has a default value: " = value) -> returnType" becomes a | ||
| /// single text token. This method splits such tokens by: | ||
| /// 1. Repeatedly extracting splittables from the start | ||
| /// 2. For the remaining content, pull splittables from the end | ||
| /// 3. Store removed suffix tokens and yield them in reverse order | ||
| /// 4. Yield any remaining non-splittable content as a single text token | ||
| /// | ||
| /// This approach preserves the correct token sequence and ensures that even | ||
| /// complex cases like " = foo) -> " are properly tokenized. | ||
| @visibleForTesting | ||
| static Iterable<Json> splitToken(Json token) sync* { | ||
| const splittables = ['(', ')', '?', ',', '->']; | ||
| const splittables = ['(', ')', '?', ',', '->', '=']; | ||
| Json textToken(String text) => | ||
| Json({'kind': 'text', 'spelling': text}, token.pathSegments); | ||
|
|
||
| final text = getSpellingForKind(token, 'text')?.trim(); | ||
| if (text == null) { | ||
| // Not a text token. Pass it though unchanged. | ||
| // Not a text token. Pass it through unchanged. | ||
| yield token; | ||
| return; | ||
| } | ||
|
|
||
| if (text.isEmpty) { | ||
| // Input text token was nothing but whitespace. The loop below would yield | ||
| // nothing, but we still need it as a separator. | ||
| // Input text token was nothing but whitespace. We still need it as a | ||
| // separator for the parser. | ||
| yield textToken(text); | ||
| return; | ||
| } | ||
|
|
||
| var suffix = text; | ||
| var remaining = text; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| while (true) { | ||
| var any = false; | ||
| for (final prefix in splittables) { | ||
| if (suffix.startsWith(prefix)) { | ||
| yield textToken(prefix); | ||
| suffix = suffix.substring(prefix.length).trim(); | ||
| any = true; | ||
| var foundPrefix = false; | ||
| for (final splittable in splittables) { | ||
| if (remaining.startsWith(splittable)) { | ||
| yield textToken(splittable); | ||
| remaining = remaining.substring(splittable.length).trim(); | ||
| foundPrefix = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!any) { | ||
| // Remaining text isn't splittable. | ||
| if (suffix.isNotEmpty) yield textToken(suffix); | ||
| break; | ||
|
|
||
| if (foundPrefix) continue; | ||
|
|
||
| // No more prefix splittables found; extract any trailing ones. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| // We collect them in a list and yield in reverse order to maintain | ||
| // the original token sequence. | ||
| final trailingTokens = <String>[]; | ||
| var currentSuffix = remaining; | ||
| while (currentSuffix.isNotEmpty) { | ||
| var foundSuffix = false; | ||
| for (final splittable in splittables) { | ||
| if (currentSuffix.endsWith(splittable)) { | ||
| trailingTokens.add(splittable); | ||
| currentSuffix = currentSuffix | ||
| .substring(0, currentSuffix.length - splittable.length) | ||
| .trim(); | ||
| foundSuffix = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!foundSuffix) break; | ||
| } | ||
|
|
||
| // Yield the core content, then trailing splittables in reverse order. | ||
| if (currentSuffix.isNotEmpty) yield textToken(currentSuffix); | ||
| for (var i = trailingTokens.length - 1; i >= 0; i--) { | ||
| yield textToken(trailingTokens[i]); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,11 +146,91 @@ ParsedFunctionInfo parseFunctionInfo( | |
| final (type, remainingTokens) = parseType(context, symbolgraph, tokens); | ||
| tokens = remainingTokens; | ||
|
|
||
| // Optional: parse a default argument if present. | ||
| // Swift symbol graph can emit defaults in these formats: | ||
DPrakashhh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 1. Separate tokens: [": ", type..., " = ", value, ", "] | ||
| // 2. Combined: [":", type..., " = value, "] or | ||
| // [":", type..., " = value)"] | ||
| // 3. With return: [":", type..., " = value) -> "] | ||
| // | ||
| // We carefully preserve string literals during parsing to avoid | ||
| // splitting on delimiters that appear inside quoted strings | ||
| // (e.g., the ") -> " in the string literal "World ) -> "). | ||
| String? defaultValue; | ||
| String? endToken; | ||
| var afterType = maybeConsume('text'); | ||
|
|
||
| if (afterType != null) { | ||
| // Check if this text token contains '=' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 If things work out like I'm expecting, then this whole chunk of code will be waaaay simpler after that rewrite. |
||
| var remaining = afterType.trim(); | ||
| if (remaining.startsWith('=')) { | ||
| // Extract default value from this token | ||
| remaining = remaining.substring(1).trim(); | ||
|
|
||
| // Check for delimiters: ',' or ')' (possibly followed by ' ->') | ||
| if (remaining.contains(')')) { | ||
| final parenIndex = remaining.indexOf(')'); | ||
| defaultValue = remaining.substring(0, parenIndex).trim(); | ||
| endToken = ')'; | ||
| } else if (remaining.contains(',')) { | ||
| final commaIndex = remaining.indexOf(','); | ||
| defaultValue = remaining.substring(0, commaIndex).trim(); | ||
| endToken = ','; | ||
| } else if (remaining.isNotEmpty) { | ||
| // Default value but no delimiter yet | ||
| defaultValue = remaining; | ||
| endToken = maybeConsume('text'); | ||
| } else { | ||
| // '=' alone, collect tokens until delimiter | ||
| final parts = <String>[]; | ||
| while (tokens.isNotEmpty) { | ||
| final tok = tokens[0]; | ||
| final kind = tok['kind'].get<String?>(); | ||
| final spelling = tok['spelling'].get<String?>(); | ||
| if (spelling != null) { | ||
| final s = spelling.trim(); | ||
| // String tokens should be kept whole, not split on delimiters | ||
| if (kind == 'string') { | ||
| parts.add(spelling); | ||
| tokens = tokens.slice(1); | ||
| } else if (s == ',' || s == ')') { | ||
| endToken = s; | ||
| tokens = tokens.slice(1); | ||
| break; | ||
| } else if (s.contains(',') || s.contains(')')) { | ||
| final delimChar = s.contains(',') ? ',' : ')'; | ||
| final delimIndex = s.indexOf(delimChar); | ||
| parts.add(s.substring(0, delimIndex).trim()); | ||
| endToken = delimChar; | ||
| tokens = tokens.slice(1); | ||
| break; | ||
| } else { | ||
| parts.add(spelling); | ||
| tokens = tokens.slice(1); | ||
| } | ||
| } else { | ||
| tokens = tokens.slice(1); | ||
| } | ||
| } | ||
| if (parts.isNotEmpty) { | ||
| defaultValue = parts.join('').trim(); | ||
| } | ||
| } | ||
| } else { | ||
| endToken = afterType; | ||
| } | ||
| } | ||
|
|
||
| parameters.add( | ||
| Parameter(name: externalParam, internalName: internalParam, type: type), | ||
| Parameter( | ||
| name: externalParam, | ||
| internalName: internalParam, | ||
| type: type, | ||
| defaultValue: defaultValue, | ||
| ), | ||
| ); | ||
|
|
||
| final end = maybeConsume('text'); | ||
| final end = endToken ?? maybeConsume('text'); | ||
| if (end == ')') break; | ||
| if (end != ',') throw malformedInitializerException; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,28 +81,29 @@ ClassDeclaration transformCompound( | |
| .nonNulls | ||
| .toList(); | ||
|
|
||
| final transformedInitializers = originalCompound.initializers | ||
| .map( | ||
| (initializer) => transformInitializer( | ||
| initializer, | ||
| wrappedCompoundInstance, | ||
| parentNamer, | ||
| state, | ||
| ), | ||
| ) | ||
| .toList(); | ||
| final transformedInitializers = <Declaration>[]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| for (final init in originalCompound.initializers) { | ||
| transformedInitializers.addAll( | ||
| transformInitializerWithOverloads( | ||
| init, | ||
| wrappedCompoundInstance, | ||
| parentNamer, | ||
| state, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| final transformedMethods = originalCompound.methods | ||
| .map( | ||
| (method) => transformMethod( | ||
| method, | ||
| wrappedCompoundInstance, | ||
| parentNamer, | ||
| state, | ||
| ), | ||
| ) | ||
| .nonNulls | ||
| .toList(); | ||
| final transformedMethods = <MethodDeclaration>[]; | ||
| for (final method in originalCompound.methods) { | ||
| transformedMethods.addAll( | ||
| transformMethodWithOverloads( | ||
| method, | ||
| wrappedCompoundInstance, | ||
| parentNamer, | ||
| state, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| transformedCompound.properties = | ||
| transformedProperties.whereType<PropertyDeclaration>().toList() | ||
|
|
||
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
returnTypecan be part of the token. If it can, then just taking splittables of the start and end won't work.