swift-syntax icon indicating copy to clipboard operation
swift-syntax copied to clipboard

Backticks on identifiers should be separate tokens

Open grynspan opened this issue 2 years ago • 16 comments

Description

If I have an expression like so:

let x = y.`z`()

Then the syntax node representing z contains the surrounding backticks. I then need to strip these off manually to get at the actual symbol name so I can use it in other contexts like context.makeUniqueName() or diagnostic messages. These backticks should probably be represented by separate tokens, no?

Steps to Reproduce

No response

grynspan avatar Jul 21 '23 18:07 grynspan

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

ahoppen avatar Jul 21 '23 18:07 ahoppen

Or perhaps they should be excluded from .text. Not sure the ideal approach here, but having to manually filter them is slowly driving me to madness. :)

grynspan avatar Jul 21 '23 19:07 grynspan

It makes sense that the backticks are part of the identifier token because the `z` token is formed in the lexer including the backticks and they thus are part of the identifier token. And we used to represent backticks as trivia at some point years ago but moved away from that for reasons that I can’t recall right now, but there were reasons.

I absolutely agree that we need to have an API on TokenSyntax that removes the trivia. We briefly touched upon this here: https://github.com/apple/swift-syntax/pull/1816#discussion_r1238428108

The suggestion is that we introduce something like the following and then you could use token.identifier.

struct Identifier {
  var name: String

  init(_ token: TokenSyntax) {
     // FIXME: Handle backticks
    self.name = token.text
  }
}

extension TokenSyntax {
  var identifier: Identifier {
    Identifier(self)
  }
}

ahoppen avatar Jul 25 '23 05:07 ahoppen

Oh, sure—I am maybe fixating on a specific technical solution here. Feel free to come up with a better idea! :)

grynspan avatar Jul 25 '23 13:07 grynspan

This looks interesting!

I'm currently looking at macros and getting in to swift syntax and would love to contribute my first issue/fix and this one seems a nice isolated one to kick things off

Aware this is 8 months ago so things might have changed but sounds like this is all still viable as above if I were to look in to it? 👨🏻‍💻

adammcarter avatar Mar 27 '24 23:03 adammcarter

Yes, the issue is still open and still applies.

ahoppen avatar Mar 28 '24 15:03 ahoppen

I'm just looking in to this now, from what I can tell from this issue and the one linked by @ahoppen here: https://github.com/apple/swift-syntax/pull/1816#discussion_r1238428108 ...

It looks like the criteria here is:

  1. Avoid using backticks as their own trivia

we used to represent backticks as trivia at some point years ago but moved away from that for reasons that I can’t recall right now, but there were reasons.

  1. Create a new Identifier type that extracts backticks when creating the name (as per the example here https://github.com/apple/swift-syntax/issues/1936#issuecomment-1649144729)

I'm going to look at this now and see how I get on

adammcarter avatar Mar 28 '24 15:03 adammcarter

Hey @ahoppen / @grynspan 👋🏻

I have a branch with the above changes and soon I'll be ready to push but I don't yet have permissions - is someone able to help out? Thanks!

adammcarter avatar Mar 28 '24 18:03 adammcarter

What you should do is fork the repository (via GitHub) to your account, then move the branch to your fork. From there, GitHub will let you open a PR back to the original repo.

If that's not working, we can look into it.

grynspan avatar Mar 28 '24 18:03 grynspan

Hi there! It's fantastic to hear that you've prepared changes to contribute!

To submit a Pull Request, you don't need permissions. Here's a quick guide on how to proceed:

  1. Fork this repository.
  2. Clone your fork to your local machine.
  3. Create a new branch in your local repository.
  4. Commit your changes to this new branch.
  5. Push the branch to the origin of your fork.

Once you've pushed your branch, you should see an option on this repository's page to create a PR from a branch in your fork.

For more detailed guidance on contributing to this project, I recommend checking out these resources:

Thanks for your contribution!

Matejkob avatar Mar 28 '24 18:03 Matejkob

Amazing thanks both! 👌🏻

Apologies, I did read the contributing guides but didn't see anything re forking the repo. Did I scroll straight past it and it was right in front of me? Or are these steps not included?

If it's the latter, is it worth putting those instructions in that CONTRIBUTING guide @Matejkob ?

adammcarter avatar Mar 28 '24 18:03 adammcarter

You're welcome, and no worries at all!

To the best of my knowledge, you're right: neither of the documents explicitly mentions the step about forking the repository. I included those links thinking they might offer additional insights on other aspects of contributing, not realizing the forking step wasn't covered. My apologies for any confusion that may have caused.

It's a good suggestion to add a section about forking to the CONTRIBUTING guide.

Matejkob avatar Mar 28 '24 18:03 Matejkob

Can you file an issue against the Swift repo about that oversight? Thanks! 😄

grynspan avatar Mar 28 '24 18:03 grynspan

No worries at all!

Issue opened below for the following repos:

  • swift-syntax: https://github.com/apple/swift-syntax/issues/2575
  • swift: https://github.com/apple/swift/issues/72674

adammcarter avatar Mar 28 '24 19:03 adammcarter

For completeness sakes, PR for this issue is opened here: https://github.com/apple/swift-syntax/pull/2576

adammcarter avatar Mar 28 '24 19:03 adammcarter

Can you file an issue against the Swift repo about that oversight? Thanks! 😄

@grynspan PR here https://github.com/apple/swift/pull/72731

adammcarter avatar Mar 31 '24 20:03 adammcarter

Closing as completed by https://github.com/apple/swift-syntax/pull/2576.

ahoppen avatar Jun 03 '24 17:06 ahoppen