Skip to content
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

indexInParent replacement for use with replacing(childAt #1872

Closed
keith opened this issue Jun 30, 2023 · 5 comments
Closed

indexInParent replacement for use with replacing(childAt #1872

keith opened this issue Jun 30, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@keith
Copy link
Member

keith commented Jun 30, 2023

Description

I had some SwiftSyntax code that did:

override func visit(_ node: ImportDeclSyntax) -> DeclSyntax {
    guard let importAccessPathSyntax = node.path.first(where: { $0.name.text.contains("Foo") }) {
        return super.visit(node)
    }

    let newToken = importAccessPathSyntax.name.withKind(.stringSegment("Bar"))
    let newPath = node.path.replacing(childAt: importAccessPathSyntax.indexInParent,
                                        with: importAccessPathSyntax.with(\.name, newToken))
    let newNode = node.with(\.path, newPath)
    correctionPositions.append(importAccessPathSyntax.positionAfterSkippingLeadingTrivia)
    return super.visit(newNode)
}

In order to replace imports of Foo with Bar. I was able to do this without affecting any trivia using this approach, but it seems like with recent changes on the release/5.9 branch there is no way to get the equivalent of importAccessPathSyntax.indexInParent anymore. I attempted to use importAccessPathSyntax.keyPathInParent as a replacement here but in this case that results in nil. Should that be the new option? Should replacing(childAt: take SyntaxChildrenIndex now instead of an Int? In the meantime I worked around this case by assuming that 0 was the right index always:

override func visit(_ node: ImportDeclSyntax) -> DeclSyntax {
    guard let importAccessPathSyntax = node.path.first(where: { $0.name.text.contains("Foo") }) {
        return super.visit(node)
    }

    let newToken = importAccessPathSyntax.name.with(\.tokenKind, TokenKind.stringSegment("Bar"))
    let newPath = node.path.replacing(childAt: 0, with: importAccessPathSyntax.with(\.name, newToken))
    let newNode = node.with(\.path, newPath)
    correctionPositions.append(importAccessPathSyntax.positionAfterSkippingLeadingTrivia)
    return super.visit(newNode)
}
@keith keith added the enhancement New feature or request label Jun 30, 2023
@ahoppen
Copy link
Member

ahoppen commented Jun 30, 2023

Tracked in Apple’s issue tracker as rdar://111591397

@ahoppen
Copy link
Member

ahoppen commented Jul 6, 2023

You should be able to get the index of the element you want to replace when finding the element that contains “Foo”. This is similar to what you would do in a normal Swift array as well.

override func visit(_ node: ImportDeclSyntax) -> DeclSyntax {
    guard let (index, importAccessPathSyntax) = node.path.enumerated().first(where: { $0.element.name.text.contains("Foo") }) else {
      return super.visit(node)
    }

    let newToken = importAccessPathSyntax.name.with(\.tokenKind, .stringSegment("Bar"))
    let newPath = node.path.replacing(childAt: index, with: importAccessPathSyntax.with(\.name, newToken))
    let newNode = node.with(\.path, newPath)
    correctionPositions.append(importAccessPathSyntax.positionAfterSkippingLeadingTrivia)
    return super.visit(newNode)
}

Thinking about this a little bit more, though, replacing(childAt:with:) probably shouldn’t exist. Instead we should make the subscript on SyntaxCollection writable. I’ll update once I have thought about this a little bit more.

@Matejkob
Copy link
Contributor

I'm wondering if the issue was completely resolved in #1880?

@ahoppen
Copy link
Member

ahoppen commented Jul 17, 2023

I still have two small small items on my to-do list to resolve this. I think I should get to them this week. So: no, not quite resolved yet.

@ahoppen
Copy link
Member

ahoppen commented Aug 7, 2023

Closing this now that we have #1919. I also wanted to do #1921 as part of this but it has SILGen issues on Windows that I need to investigate and it’s not really necessary to resolve this issue.

@ahoppen ahoppen closed this as completed Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants