Skip to content

[SwiftParser] Experimental feature flags for _borrow and _move #3018

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
Mar 17, 2025

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Mar 16, 2025

_borrow and _move should be parsed as an ownership operator only when a feature flag is enabled.

@rintaro
Copy link
Member Author

rintaro commented Mar 16, 2025

swiftlang/swift#80046
@swift-ci Please test

3 similar comments
@rintaro
Copy link
Member Author

rintaro commented Mar 16, 2025

swiftlang/swift#80046
@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Mar 17, 2025

swiftlang/swift#80046
@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Mar 17, 2025

swiftlang/swift#80046
@swift-ci Please test

_borrow and _move should be pased as and ownership operator only when a
feature flag is enabled.
@rintaro rintaro force-pushed the old-ownership-spellings branch from d1efcb9 to 64589f7 Compare March 17, 2025 16:11
@rintaro
Copy link
Member Author

rintaro commented Mar 17, 2025

swiftlang/swift#80046
@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Mar 17, 2025

swiftlang/swift#80046
@swift-ci Please test Windows

case (._borrow, let handle)?:
assert(self.experimentalFeatures.contains(.oldOwnershipOperatorSpellings))
Copy link
Member

Choose a reason for hiding this comment

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

Could you make these preconditions?

/// How to choose `assert` vs. `precondition`:
/// - Wherever possible, it is preferable to emit a diagnostic instead of
/// using `precondition`. This way the parser won't crash if the condition is
/// violated.
/// - If you think the diagnostic added above should never be emitted, it is
/// fine to also emit an `assertionFailure` in debug builds to make it easier
/// to debug the unexpected diagnostic.
/// - If in doubt always use `precondition`
/// - `assert` should only be used if checking the assertion has a non-trivial
/// cost and provides little benefit in terms of safety in release builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure it worth, although it trivial, The branch is a result of the subject expression, where the experimentalFeatures is handled, which is close enough for not checking seriously. Also even if somehow we enter this branch without oldOwnershipOperatorSpellings feature in release builds, we still have atContextualExpressionModifier() condtion check below so the check is just to make sure the condition check in ExpressionModifierKeyword.init(lexeme:) is working.

FWIW, looking at the existing code, do expression uses precodition but then statement uses assert.

@rintaro rintaro merged commit a109307 into swiftlang:main Mar 17, 2025
24 checks passed
@rintaro rintaro deleted the old-ownership-spellings branch March 17, 2025 23:49
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