Add gyb for Punctuators and Misc tokens

[kimdv]

From #263

1. Generate cases for the remaining tokens

After a quick scan through Token.py, all that we have left still are Punctuators and the Misc tokens. I don’t see a general pattern which of these need a trailing space, so we’d need to annotate them manually. For that, I’d add another property to the Python Token class, like requires_trailing_space. For keywords this is always set to true and for all other tokens, we need to specify it manually in the definition (e.g. { requires a trailing space, but . does not). On the Swift side, we can check for this property and call withTrailingTrivia or not.

I have added tokens for Punctuators and Misc, but as you mention we should add requires_trailing_space .
I have seen that the = also require a leading space, so we should also we also add an requires_leading_space for those few cases?

You mention that we should add to Token.
Is that an extension or should we add this to the main swift repo? 😁

[ktoso]

I noticed the actor keyword is not in the generated list here, does the gyb generation perhaps have to run with -enable-experimental-concurrency or is there some other way to get that?

[ahoppen]

I have added tokens for Punctuators and Misc, but as you mention we should add requires_trailing_space .
I have seen that the = also require a leading space, so we should also we also add an requires_leading_space for those few cases?

Ah, yes, forgot about = (or spaced binary operators in general which should also have a leading space). Also adding a requires_leading_space property sounds good 👍

You mention that we should add to Token.
Is that an extension or should we add this to the main swift repo? 😁

Add it to the main swift repo, set up a PR there for your changes and just link to this PR in the description.

[ahoppen]

I noticed the actor keyword is not in the generated list here, does the gyb generation perhaps have to run with -enable-experimental-concurrency or is there some other way to get that?

That’s because actor is not in the list of tokens in Token.py. I think that should be resolved in the main Swift repo and is outside the scope of this PR.

[kimdv]

@ahoppen I opened a PR in the Swift repo.
I'm not completely sure how and where we should add all those cases.

Should there be some default in the __init__ and if there is some, that need another way we can add it in the SYNTAX_TOKENS list for that special case?

Etc: Punctuator have default leading = false and trailing = true.

Punctuator('Equal', 'equal', text='=', requires_leading_space=True, serialization_code=86), and Punctuator('Period', 'period', text='.', requires_trailing_space=False, serialization_code=85),

[ahoppen]

At a first glance the Swift PR looks very good. I think you went a very good way with the defaults already.

I think for all the other tokens we can reasonably assume a default of False in the initialiser and only add requires_leading_space=True (or requires_trailing_space=True) for those tokens that require it, just like you suggested (although I think that = should also have a trailing space 😉).

If you don’t know what some of the tokens are or whether they need trivia, you can also just leave them unspaced for now. It’s probably easier to go and revisit the decisions later once we have the builder up and running and get a feeling for how it behaves.

[kimdv]

I have now pushed a new gyb file for Tokens.
I think we can make it to one loop as the Token.py contains all information about leading/trailing.

As you say we can leave them unspaced, and then change later if needed.
I have tried to find the ones that need a space.

[ktoso]

That’s because actor is not in the list of tokens in Token.py. I think that should be resolved in the main Swift repo and is outside the scope of this PR.

Okey so I confirmed that piece. We indeed have it as a contextual keyword, same with await and async.

For the builders once we get there it'll be nice to construct an actor the same way as just a class, but that's later I suppose - the actual builder DSL :)

[ahoppen]

The PR looks very good overall (also the Swift side, I’ve left a comment there as well).

I think we can make it to one loop as the Token.py contains all information about leading/trailing.

I agree that one loop should be sufficient. As a bonus you could adjust the comment to say "the … keyword" or "the … token" depending on whether the token is a keyword or not, but I’m also fine with just having the comment say "the … token".
Also, I don’t care about the MARK comments. Feel free to remove them if they complicate things.

Other than that, I’ve got a few minor comments about the doc-comments, although they are not critical. Once we generate all tokens in one loop, I think we’re good to merge.

[ahoppen]

Okey so I confirmed that piece. We indeed have it as a contextual keyword, same with await and async.

For the builders once we get there it'll be nice to construct an actor the same way as just a class, but that's later I suppose - the actual builder DSL :)

Sounds like a case where we need to hand write a constructor for those contextual keywords. We can do that once we’re approaching a close with the auto-generated stuff.

[ahoppen]

I don’t think this compiles. makeIntegerLiteral has a required text parameter.

Stupid question but are you checking if your code compiles before pushing the changes?

[kimdv]

Note to my self: Remember to delete DerivedData when chugging toolchain, sorry about that! 😞
SwiftSyntax started not to compile.. Strange.. But it should be fixed now and build and tests pass locally.

Pushed to swiftlang/swift#36383 as tests did fail with { and } because we added trailing space. We can not generalise that.

[ahoppen]

Looking good now!

[ahoppen]

swiftlang/swift#36383

@swift-ci Please test