Skip to content

Add asserts in init methods #341

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

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Nov 27, 2021

The idea is to add checks to help us and other developers to make valid swift code.

This only add for text_choices and token_choices.
I have not figured out how to add for node_choices, but this is a start.

swiftlang/swift#40359

@kimdv kimdv force-pushed the kimdv/add-asserts-in-inits branch from 7762449 to 1a2c698 Compare November 27, 2021 19:17
Comment on lines 144 to 167
% # some cases, like IdentifierToken, will add an empty string.
% # So if there is any empty string we just make it empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand your comment correctly, we currently set a Token’s text to "" if it essentially is a wildcard token and doesn’t have a fixed text, correct?

Since we now start relying on that behavior, I think it would be good if we document it in Token.py. E.g.

If `text` is not empty, a token of this type must always contain the specified `text`. If `text` is empty, the token can contain arbitrary text.

And while we’re at it, I think it would be nicer if we could use None instead of an empty string to denote the behavior that a token can contain arbitrary text – not sure what other changes that entails though.


Also a suggestion to make this comment clearer:

If a Token has an empty text, it can contain arbitrary text. If any of the token choices may contain arbitrary text, set assert_choices to [] so we don’t assert on the token’s text below.

Copy link
Contributor Author

@kimdv kimdv Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the text in a Token is set to "" if it is None.
https://github.com/apple/swift/blob/f00f2841abf79b81a20b01936137eb095bd9b1df/utils/gyb_syntax_support/Token.py#L21

I agree that it would be more nice with an None instead of an "" in Token.

Added the comment above to Token.text
swiftlang/swift#40359

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have run degyb locally where Token.text is None. Thus removed or "".
It didn't generate any other files?

Are there any other repos depending on it?
I could push and we could try to run CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not entirely if I understood your questions correctly, but I’m trying to answer as best as I can.

I have run degyb locally where Token.text is None. Thus removed or "".
It didn't generate any other files?

It could very well be that changing Token.text from "" to None doesn’t change any of the generated files in the SwiftSyntax repo, so this might be expected.

Are there any other repos depending on it?
gyb_syntax_support is only used by the compiler itself (in include/swift/Syntax and lib/Syntax) and SwiftSyntax. So any breakage will be found by CI.

I could push and we could try to run CI?
Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI did pass 🥳

@kimdv kimdv force-pushed the kimdv/add-asserts-in-inits branch 3 times, most recently from 8c7d7cd to 14f3604 Compare December 1, 2021 20:16
@kimdv kimdv force-pushed the kimdv/add-asserts-in-inits branch from 14f3604 to 4f43f45 Compare December 2, 2021 12:03
@kimdv
Copy link
Contributor Author

kimdv commented Dec 2, 2021

swiftlang/swift#40359

@swift-ci Please test

@kimdv kimdv requested a review from ahoppen December 2, 2021 17:00
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small comment nitpick. Otherwise 👍

@kimdv kimdv force-pushed the kimdv/add-asserts-in-inits branch from 4f43f45 to 266094c Compare December 2, 2021 18:53
@kimdv kimdv force-pushed the kimdv/add-asserts-in-inits branch from 266094c to fa6658b Compare December 2, 2021 18:53
@kimdv
Copy link
Contributor Author

kimdv commented Dec 2, 2021

swiftlang/swift#40359

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2021

Build failed
Swift Test OS X Platform
Git Sha - 4f43f45

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2021

Build failed
Swift Test Linux Platform
Git Sha - 4f43f45

@kimdv kimdv merged commit 8b60985 into swiftlang:main Dec 3, 2021
@kimdv kimdv deleted the kimdv/add-asserts-in-inits branch December 3, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants