Implement StringLiteralExprSyntax/contentValue. #1508
Conversation
| /// | ||
| /// Returns nil if the literal contains interpolation segments. | ||
|
|
||
| public var contentValue: String? { |
There was a problem hiding this comment.
We need to be very very careful about trafficking in Strings derived from the syntax tree. The documentation should be very clear about the limitations of this API with respect to the caveats in its implementation and the source fidelity concerns laid out here.
There was a problem hiding this comment.
I agree that contentValue sounds a little like the actual string content of the literal. What do you think about unescapedValue? I am not terribly happy with the name and am open to better suggestions.
There was a problem hiding this comment.
I had trouble coming up with a proper name.
I considered a couple of candidates like stringValue, runtimeValue, content, etc. The final decision for contentValue came from some existing code in ConvenienceInitializers.swift which basically does the opposite: Building a StringLiteralExprSyntax from an unescaped String value. This initializer uses the label content: for the value. Just content seemed to conflicting, though.
I'm also open for better names. I have the feeling unescapedValue only matches partly, leaving out the concatenation part, and is difficult to find. But I'll follow your advice here.
There was a problem hiding this comment.
I think the main reason why I don’t like contentValue is that content is already a term that’s used in SwiftSyntax, e.g. StringLiteralExprSyntax has a contentLength property that’s completely unrelated to contentValue. And that’s pretty confusing to me.
Alternative idea: What do you think about representedLiteralValue?
|
I'm curious why this in |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks, this is looking great to me. A very clever use of the lever to lex the escaped string characters.
@CodaFi IIUC this lives in SwiftParser because it access the lever, which isn’t public (and shouldn’t be). We could define this as SPI in SwiftParser and then offer it as public API in SwiftSyntaxBuilder but I think it shouldn’t live in SwiftSyntaxBuilder because it’s not about building a syntax tree. So, I think SwiftParser is indeed the correct location for this property.
| /// | ||
| /// Returns nil if the literal contains interpolation segments. | ||
|
|
||
| public var contentValue: String? { |
There was a problem hiding this comment.
I agree that contentValue sounds a little like the actual string content of the literal. What do you think about unescapedValue? I am not terribly happy with the name and am open to better suggestions.
|
|
||
| /// Helper class to find string literals in this source file | ||
| class StringLiteralCollector: SyntaxVisitor { | ||
| var locationConverter: SourceLocationConverter? = nil |
There was a problem hiding this comment.
It's only used temporarily during walk and can be freed afterwards. Not sure if this is a good reason.
My understanding of SwiftSyntax' module structure is limited. But I think it could actually live in the plain SwiftSyntax module. The functionality is neither about parsing nor building but really just a derived value from the syntax node. It could potentially be useful from manually initialized nodes. The reason I put it in SwiftParser is because I'm reusing private API from the parser module: |
|
I'm not sure about the proper flow: Shall I squash the two commits? Or shall I even rebase onto current main? How does the PR proceed from here? Thanks, again! |
| /// resolved. | ||
| /// | ||
| /// Returns nil if the literal contains interpolation segments. | ||
|
|
| /// | ||
| /// Most tests are expressed by a single function call that compares the | ||
| /// run-time String value against the parsed node's `contentValue`. | ||
|
|
| /// | ||
| /// Returns nil if the literal contains interpolation segments. | ||
|
|
||
| public var contentValue: String? { |
There was a problem hiding this comment.
I think the main reason why I don’t like contentValue is that content is already a term that’s used in SwiftSyntax, e.g. StringLiteralExprSyntax has a contentLength property that’s completely unrelated to contentValue. And that’s pretty confusing to me.
Alternative idea: What do you think about representedLiteralValue?
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for the revision. I think the only thing remaining is the naming of the method. But as they say: The 2 hardest problems in computer science are cache invalidation, naming and off-by-1 errors 😆
Regarding the process: There’s no need to rebase unless there are merge conflicts. Since we don’t squash commits on merge, it would be great if you could squash your commits. Once we’ve decided on the name, I’ll run CI and if that passes, I’ll merge the PR.
NikolaiRuhe
force-pushed
the
string-literal-value
branch
from
11ec8d5 to
19c7184
Compare
|
Yes, naming indeed can be very difficult—and is at the same time so important. It's a bit like an app's icon on the app store. The first thing you see and it should be able to explain what it does in a glimpse. Nobody reads the docs 😢. In that sense I do like Thanks for the help and the review! |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Great. Thank you. Let’s merge it.
|
@swift-ci Please test |
|
@swift-ci please test windows |
4 participants
SwiftSyntax parses string literals as
StringLiteralExprSyntax. When the literal is static (does not contain interpolation segments) it can be useful to access the actual string value from the syntax tree.The source-accurate representation in
StringLiteralExprSyntaxdiffers from run-time value:\n,\u{1F600}) are not evaluated.StringLiteralExprSyntax/contentValueperforms the necessary unescaping and concatenation of segments.