Skip to content

Enable assertions in CI even if we are building in release builds #1342

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.

Sign up for GitHub

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

Jump to bottom
Merged
ahoppen merged 1 commit into from
Mar 23, 2023
Merged

Enable assertions in CI even if we are building in release builds #1342

ahoppen merged 1 commit into from
Mar 23, 2023

Conversation

@ahoppen
Copy link
Member

@ahoppen ahoppen commented Feb 13, 2023 •
edited
Loading

This overrides the standard assert function from the stdlib with a version also checks the condition in CI if a conditional compilation flag is passed.

rdar://106874411

@ahoppen
Copy link
Member Author

ahoppen commented Feb 13, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/always-assert branch 2 times, most recently from d272fed to c3ec859 Compare February 14, 2023 11:25
@ahoppen
Copy link
Member Author

ahoppen commented Feb 14, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from c3ec859 to 29987c0 Compare March 14, 2023 00:29
@ahoppen
Copy link
Member Author

ahoppen commented Mar 14, 2023

@swift-ci Please test

@ahoppen ahoppen changed the title Enable assertions in release builds Enable assertions in CI even if we are building in release builds Mar 17, 2023
@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from 29987c0 to 91f10e7 Compare March 17, 2023 17:24
@ahoppen
Copy link
Member Author

ahoppen commented Mar 17, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from 91f10e7 to ebef3e5 Compare March 17, 2023 20:13
@ahoppen
Copy link
Member Author

ahoppen commented Mar 17, 2023

@swift-ci Please test

@kimdv
Copy link
Contributor

kimdv commented Mar 18, 2023

@swift-ci please test Windows

@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from ebef3e5 to 45134da Compare March 22, 2023 20:46
@ahoppen
Copy link
Member Author

ahoppen commented Mar 22, 2023

Rationale behind whether we should override the assert and or precondition from the stdlib since all packages that import SwiftSynax will receive the versions defined in this package instead of the one from the standard library:

  • It is OK to override assert because it transparently forwards to Swift.assert unless the SWIFTSYNTAX_ENABLE_ASSERTIONS conditional compilation flag is passed, which should never be the case outside of SwiftSyntax’s CI
  • We considered overriding precondition with a version that prints the failure message when the precondition is violated but decided against it because
    • This would subtly change how precondition works once packages import SwiftSyntax
    • We believe there is not too much value in outputting the failure message of preconditions in SwiftSyntax since most failure messages are static anyway.

@ahoppen
Copy link
Member Author

ahoppen commented Mar 22, 2023

@swift-ci Please test

bnbarham
bnbarham approved these changes Mar 22, 2023
View reviewed changes
Sources/SwiftSyntax/Assert.swift Outdated
@_transparent
public func assert(_ condition: @autoclosure () -> Bool, _ message: @autoclosure () -> String = String(), file: StaticString = #file, line: UInt = #line) {
#if SWIFTSYNTAX_ENABLE_ASSERTIONS
precondition(condition(), message(), file: file, line: line)
Copy link
Contributor

@bnbarham bnbarham Mar 22, 2023

Choose a reason for hiding this comment

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

You could change this to fatalError rather than precondition so we get messages still. Or not, probably doesn't really matter.

bnbarham
bnbarham approved these changes Mar 22, 2023
View reviewed changes
utils/group.json
"TokenDiagnostic.swift",
],
"Internal": [
"Assert.swift",
Copy link
Contributor

@bnbarham bnbarham Mar 22, 2023

Choose a reason for hiding this comment

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

Needs to be added to CMakeLists.txt as well

@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from 45134da to b1dd041 Compare March 22, 2023 21:50
@ahoppen
Copy link
Member Author

ahoppen commented Mar 22, 2023

@swift-ci Please test

@ahoppen
Enable assertions in CI even if we are building in release builds
e001fb1 
This overrides the standard `assert` function from the stdlib with a version also checks the condition in CI if a conditional compilation flag is passed.
@ahoppen ahoppen force-pushed the ahoppen/always-assert branch from b1dd041 to e001fb1 Compare March 23, 2023 00:17
@ahoppen
Copy link
Member Author

ahoppen commented Mar 23, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Mar 23, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 9a6b16b into swiftlang:main Mar 23, 2023
@ahoppen ahoppen deleted the ahoppen/always-assert branch March 23, 2023 17:30
@ahoppen ahoppen mentioned this pull request Mar 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnbarham bnbarham bnbarham approved these changes

@kimdv kimdv kimdv approved these changes

@rintaro rintaro Awaiting requested review from rintaro rintaro is a code owner

@DougGregor DougGregor Awaiting requested review from DougGregor

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ahoppen @kimdv @bnbarham