-
Notifications
You must be signed in to change notification settings - Fork 248
Add support for InlineArray type sugar #1043
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
@@ -1732,6 +1732,12 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { | |||
return .visitChildren | |||
} | |||
|
|||
override func visit(_ node: InlineArrayTypeSyntax) -> SyntaxVisitorContinueKind { | |||
before(node.separator, tokens: .space) | |||
after(node.separator, tokens: .space) |
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.
Line breaks are allowed after the of
keyword (but not before). Can you update this to allow for a break here, and add tests that verify that they wrap reasonably when horizontally constrained?
This will also help us determine if we need to add groups anywhere or if the ones already added by the type's own formatting are sufficient.
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've applied the changes and added more test cases 🙇
let a: [[3 of Int]] | ||
let a: [[Int of 3]] | ||
let a: [_ of Int] | ||
let a: [Int of Int] |
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 know that the parser technically supports parsing any type scalar before the of
for better error reporting (and to support integer generic type parameters), but can we separate the tests for the "normal" ones from the "weird" ones? It's fine to make sure that we format the otherwise incorrect cases reasonably, but I'd like for us to have a comment somewhere that explains "these aren't actually valid Swift, but the parser accepts them so we don't want to make them worse if someone typed it and then formatted it".
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 included all the expressions from the swift-syntax tests, which ended up pulling in some weird cases as well.
To avoid potential confusion during future maintenance, how about removing the tests for the weird cases and keeping only the normal ones? 🤔
Or do you think it might be better to keep the tests even if the code isn’t actually valid?
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.
Removing them is fine too. Just keep one that doesn't use an integer literal around (e.g., [n of Int]
), since that's valid as long as the parent generic context has <let n: Int>
.
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.
Understood. Thank you for the helpful feedback!
e089934
to
fe9df7d
Compare
""" | ||
|
||
let expected = |
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.
Hmm, the last two of these look incorrect. I would expect the following:
let a:
[3 of
VeryLongGenericTypeNameThatCausesWrapping]
let a:
[3 of
[3 of
VeryLongGenericTypeNameThatCausesWrapping]]
// ^ needed to be indented more
let a =
[
3 of
VeryLongGenericTypeNameThatCausesWrapping
](
repeating:
foo
)
The first two are still a little weird, but is similar to how we handle ArrayTypeSyntax
—that is, we don't handle it at all and just let whatever's inside it determine how it wraps. That means that even outside of inline array sugar, the thing inside the brackets in let a: [LongThing]
wraps very differently from let a = [LongThing]()
. Let's ignore that for now.
The last one is sort of tricky, but we need that line break before the ]
in that case to satisfy our other rules about how continuation lines force a break if there would be an indented line following it.
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.
Array types are parsed as ArrayTypeSyntax
when used in type declarations, and as ArrayExprSyntax
when used with initializers.
This causes formatting differences between let a: [LongThing]
and let a = [LongThing]()
.
On the other hand, InlineArray is always parsed as InlineArrayTypeSyntax
, regardless of context.
So if we want to match the formatting behavior of regular arrays, we may need to make InlineArrayTypeSyntax
explicitly check whether its parent
conforms to CallingExprSyntaxProtocol
.
What do you think about having a consistent line-breaking behavior for inline arrays—regardless of where they’re used—like in the latest commit?
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.
Yeah, I suppose that's a reasonable default for now. It's not optimal, but it'll at least always be "correct" (in terms of following our line wrapping rules), and the chances of someone having this kind of situation aren't that high. We can revisit it later with some of the other types of nodes. Thanks!
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.
The line breaks may seem excessive because the linelength
limit is set to an extremely low value (5),
but with a more reasonable setting (like 40), the output looks like this:
let a:
[3 of
VeryLongGenericTypeNameThatCausesWrapping]
let a:
[3 of
[3 of
VeryLongGenericTypeNameThatCausesWrapping]]
// ^ It still seems like we need to add some indentation here, though 😅
let a = [3 of
VeryLongGenericTypeNameThatCausesWrapping](
repeating: foo)
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.
Hmm, it looks like that with the latest commit? I wouldn't expect that—the .open
/.close
groups you added inside the brackets should always force 3 of
onto the next line if any part of that group breaks, and it should also force the closing bracket to wrap down.
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.
Ah, my apologies — I ran the tests after accidentally rolling back the commit.
It's working as expected now. Sorry for the confusion 🙇
fe9df7d
to
c2c0c4a
Compare
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.
Will keep an eye out for the dependent swift-syntax PR to merge.
""" | ||
|
||
let expected = |
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.
Yeah, I suppose that's a reasonable default for now. It's not optimal, but it'll at least always be "correct" (in terms of following our line wrapping rules), and the chances of someone having this kind of situation aren't that high. We can revisit it later with some of the other types of nodes. Thanks!
Resolve #978
Adds support for the inline array type sugar, now that it has been promoted from an experimental feature.
This change should be merged after swiftlang/swift-syntax#3125 has been merged.