Add fix and tests for postfix pound ifs #383
Conversation
There was a problem hiding this comment.
Thanks for looking into this! This looks like a great start; I've left a couple comments about extra things we should make sure to consider.
Typically we like to have changes made only on the main branch, and then we can cherry-pick them into the release branches. But the main branch has fallen behind the latest development snapshots until #384 is merged, so feel free to keep it on the 5.6 branch for now until we have that sorted out and then you can rebase it and use a snapshot toolchain to test it.
| VStack { | ||
| Text("something") | ||
| #if os(iOS) | ||
| .iOSSpecificModifier() |
There was a problem hiding this comment.
Can you add a test for a situation where you would have multiple members accesses inside the #if block? For example:
#if os(iOS)
.foo()
.bar()
#endif
We want to make sure the .bar() line (and any subsequent lines) don't get additional indentation; they should be the same as .foo().
There was a problem hiding this comment.
| if let condition = node.condition { | ||
| if let previousToken = node.previousToken, | ||
| node.condition?.nextToken?.text == ".", | ||
| (previousToken.text == ")" || previousToken.text == "}") { |
There was a problem hiding this comment.
This will handle things like initializer or builder expressions, but what about something that doesn't end in ) or } as the base expression? For example, either of these:
someLocalVar
#if os(iOS)
.modifier()
#endif
SomeType("blah")
.property
#if os(iOS)
.modifier()
#endif
Instead of looking at the token specifically, I wonder if we can glean the right information from the parent node? The IfConfigDeclSyntax should have a PostfixIfConfigExprSyntax as its parent if we want to enter this branch, so checking that might be enough (and I think that's the case for nested #ifs as well).
There was a problem hiding this comment.
I’ll check those out, thanks for the pointers!
| """ | ||
|
|
||
| assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) | ||
| } |
There was a problem hiding this comment.
A test that uses nested #ifs would be great too, since they have a somewhat interesting underlying node structure:
Text("...")
#if os(iOS) || os(watchOS)
#if os(iOS)
.iOSModifier()
#else
.watchOSModifier()
#endif
.iOSAndWatchOSModifier()
#endif
There was a problem hiding this comment.
That PR has now been merged, so if you make the requested changes and rebase on top of |
DavidBrunow
force-pushed
the
fixPostfixPoundIfs
branch
from
95ad7cf to
5ed571c
Compare
I finally found some time to work on this. I think my solution is greatly improved but I'm not sure if the output is what is expected in all cases -- I'd appreciate feedback on that. |
DavidBrunow
force-pushed
the
fixPostfixPoundIfs
branch
3 times, most recently
from
b073a8f to
9bc613e
Compare
| @@ -3460,7 +3468,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { | |||
|
|
|||
| if let calledMemberAccessExpr = calledExpression.as(MemberAccessExprSyntax.self) { | |||
| if calledMemberAccessExpr.base != nil { | |||
| before(calledMemberAccessExpr.dot, tokens: [.break(.contextual, size: 0)]) | |||
| if Syntax(calledMemberAccessExpr).isPostfix { | |||
There was a problem hiding this comment.
Without this conditional I saw the following output:
...
Text("")
#if os(iOS)
.iOSSpecificModifier()
.anotherModifier() // this line has extra indentation
#endif
.commonModifier()
...
DavidBrunow
force-pushed
the
fixPostfixPoundIfs
branch
from
9bc613e to
85c9757
Compare
| @@ -3485,6 +3497,22 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { | |||
| } | |||
| } | |||
|
|
|||
| extension Syntax { | |||
| var isPostfixIfConfig: Bool { | |||
There was a problem hiding this comment.
I'm a little wary of making this an extension on Syntax, since it makes it look like a natural API provided by swift-syntax (I know that's kind of the point of extensions, but in this case, the API is specialized enough that it doesn't feel like it should be given that kind of prominence).
Could you make this just a file-level private function that just takes the node as an argument, instead? Named something like isNestedInPostfixIfConfig (the current name reads like it's checking that the node itself is the postfix-if-config, rather than checking that it's a descendant of one).
There was a problem hiding this comment.
I pushed this change but haven't tested it yet -- I can't get main to build so I'll either need to figure that out or cherry-pick to the 5.6 branch. I hope to get to that later today.
There was a problem hiding this comment.
I just pulled it down and tested against main, and everything looks good, so I'll go ahead and merge and save you the trouble. Thanks again!
| var isPostfixIfConfig: Bool { | ||
| var this: Syntax? = self | ||
|
|
||
| while this?.parent != nil { |
There was a problem hiding this comment.
Do we need to check all the ancestors, not just the immediate parent? Is this required to make nested clauses have the right indentation? (If it is, that's fine, I just wanted to verify the reason.)
There was a problem hiding this comment.
From what I was seeing, yes we need to check all the ancestors. In the code to handle the break before the #if the ancestor was consistent but it was a great-grandparent, not the immediate parent.
But in the code to ensure that the indentation is correct the ancestor that was PostfixIfConfigExprSyntax changed based upon how many modifiers were chained together.
Add fix and tests for postfix pound ifs Co-authored-by: David Brunow <>
Fixes #303
This solution feels kind of messy and I'd be happy to clean it up some if y'all have any guidance. I branched off of
release/5.6since this issue exists there, please let me know if using a different base is better.