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

Backticks on identifiers should be separate tokens #1936

Closed
grynspan opened this issue Jul 21, 2023 · 17 comments
Closed

Backticks on identifiers should be separate tokens #1936

grynspan opened this issue Jul 21, 2023 · 17 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@grynspan
Copy link
Contributor

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 grynspan added the bug Something isn't working label Jul 21, 2023
@ahoppen
Copy link
Collaborator

ahoppen commented Jul 21, 2023

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

@grynspan
Copy link
Contributor Author

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. :)

@ahoppen
Copy link
Collaborator

ahoppen commented Jul 25, 2023

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: #1816 (comment)

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 ahoppen added good first issue Good for newcomers and removed good first issue Good for newcomers labels Jul 25, 2023
@grynspan
Copy link
Contributor Author

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

@adammcarter
Copy link
Contributor

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? 👨🏻‍💻

@ahoppen
Copy link
Collaborator

ahoppen commented Mar 28, 2024

Yes, the issue is still open and still applies.

@adammcarter
Copy link
Contributor

I'm just looking in to this now, from what I can tell from this issue and the one linked by @ahoppen here: #1816 (comment) ...

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 Backticks on identifiers should be separate tokens #1936 (comment))

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

@adammcarter
Copy link
Contributor

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!

@grynspan
Copy link
Contributor Author

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.

@Matejkob
Copy link
Contributor

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!

@adammcarter
Copy link
Contributor

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 ?

@Matejkob
Copy link
Contributor

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.

@grynspan
Copy link
Contributor Author

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

@adammcarter
Copy link
Contributor

@adammcarter
Copy link
Contributor

For completeness sakes, PR for this issue is opened here: #2576

@adammcarter
Copy link
Contributor

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

@grynspan PR here apple/swift#72731

@ahoppen
Copy link
Collaborator

ahoppen commented Jun 3, 2024

Closing as completed by #2576.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants