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

Validate token choices in CI #1466

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 29, 2023

This changes the the paradigm around token validation in SwiftSyntax. Previously, we always validated that the RawSyntax tree layout was as expected in debug builds (independent of whether we forced assertions on). This property should always be guaranteed by our API unless you are replacing children manually. Verification that the tokens in the tree matched the token choices, on the other hand, was always disabled unless you specifically enabled it by modifying Package.swift.

This changes the paradigm by enabling RawSyntax layout and token validation whenever the SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION conditional compilation flag is set. I am planning to set this compilation flag on SwiftSyntax’s PR testing jobs at first and, after considering the performance impact, also on Swift’s PR jobs. This should give us two advantages:

  • In CI we get more extensive tree validation because we are now also validate the token choices
  • If we are just building SwiftSyntax as a package dependency in SwiftPM, we get faster performance because we skip the RawSyntax layout validation (which is really just a way to debug serious issues while working on SwiftSyntax itself).

@ahoppen
Copy link
Member Author

ahoppen commented Mar 29, 2023

swiftlang/swift#64748

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/rawsyntaxvalidation branch from 2304c47 to d7d76ac Compare March 30, 2023 14:27
@ahoppen
Copy link
Member Author

ahoppen commented Mar 30, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

@ahoppen ahoppen force-pushed the ahoppen/rawsyntaxvalidation branch from d7d76ac to 8b43d61 Compare March 31, 2023 04:59
@ahoppen
Copy link
Member Author

ahoppen commented Mar 31, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

@ahoppen ahoppen force-pushed the ahoppen/rawsyntaxvalidation branch from 8b43d61 to 9018486 Compare March 31, 2023 15:20
@ahoppen
Copy link
Member Author

ahoppen commented Mar 31, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Mar 31, 2023
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Mar 31, 2023
@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

2 similar comments
@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 5, 2023
@ahoppen
Copy link
Member Author

ahoppen commented Apr 6, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

This changes the the paradigm around token validation in SwiftSyntax. Previously, we always validated that the `RawSyntax` tree layout was as expected in debug builds (independent of whether we forced assertions on). This property should always be guaranteed by our API unless you are replacing children manually. Verification that the tokens in the tree matched the token choices, on the other hand, was always disabled unless you specifically enabled it by modifying Package.swift.

This changes the paradigm by enabling `RawSyntax` layout and token validation whenever the `SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION` conditional compilation flag is set. I am planning to set this compilation flag on SwiftSyntax’s PR testing jobs at first and, after considering the performance impact, also on Swift’s PR jobs. This should give us two advantages:
- In CI we get more extensive tree validation because we are now also validate the token choices
- If we are just building SwiftSyntax as a package dependency in SwiftPM, we get faster performance because we skip the `RawSyntax` layout validation (which is really just a way to debug serious issues while working on SwiftSyntax itself).
@ahoppen ahoppen force-pushed the ahoppen/rawsyntaxvalidation branch from 9018486 to e725602 Compare April 6, 2023 16:59
@ahoppen
Copy link
Member Author

ahoppen commented Apr 6, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Apr 12, 2023

swiftlang/swift#64748

@swift-ci Please test macOS

@ahoppen
Copy link
Member Author

ahoppen commented Apr 13, 2023

swiftlang/swift#64748

@swift-ci Please test Windows

@ahoppen
Copy link
Member Author

ahoppen commented Apr 13, 2023

swiftlang/swift#64748

@swift-ci Please test Linux

@ahoppen
Copy link
Member Author

ahoppen commented Apr 13, 2023

swiftlang/swift#64748

@swift-ci Please test Windows

@ahoppen ahoppen merged commit bc156fc into swiftlang:main Apr 13, 2023
@ahoppen ahoppen deleted the ahoppen/rawsyntaxvalidation branch April 13, 2023 22:40
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
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.

1 participant