Skip to content

Introduce then statements #67454

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 2 commits into from
Sep 1, 2023
Merged

Introduce then statements #67454

merged 2 commits into from
Sep 1, 2023

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Jul 21, 2023

These allow multi-statement if/switch expression branches that can produce a value at the end by saying then <expr>. This is gated behind -enable-experimental-feature ThenStatements pending evolution discussion.

@hamishknight hamishknight force-pushed the then branch 4 times, most recently from bb194a9 to 15f68f6 Compare July 24, 2023 21:38
@hamishknight hamishknight changed the title [WIP] Try out then statements Introduce then statements Aug 4, 2023
@hamishknight hamishknight marked this pull request as ready for review August 4, 2023 21:07
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.

Looks good to me. Just a few minor comments.

Comment on lines +970 to +967
// Issue a warning when the expression is on a different line than
// the 'then' keyword, but both have the same indentation.
if (SourceMgr.getLineAndColumnInBuffer(thenLoc).second ==
SourceMgr.getLineAndColumnInBuffer(exprLoc).second) {
diagnose(exprLoc, diag::unindented_code_after_then);
diagnose(exprLoc, diag::indent_expression_to_silence);
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Would be nice if we could do the same in swift-syntax but there currently isn’t any way to emit warnings from the parser (only from the lexer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on where we land with the newline rule, we may not even need this logic in the first place

@hamishknight hamishknight force-pushed the then branch 6 times, most recently from e21c520 to 16782d7 Compare August 10, 2023 23:38
@gottesmm
Copy link
Contributor

@hamishknight I am doing something similar that is going to need to be gated by an experimental feature in SwiftSyntax. Is it possible for you to split out the addition of the experimental feature support into a separate commit in this same PR. This will make it easier for people to cargo cult this work forward by just looking at a small commit that implements this rather than the large commit here. (I am not just doing this for myself, I am thinking down the line about other people).

@gottesmm
Copy link
Contributor

Or actually from talking @ahoppen this is going to take some time for you to do. I am going to see if I can cargo cult some of your work around the experimental feature since I need the same thing for reference bindings.

@hamishknight
Copy link
Contributor Author

@gottesmm Hey Michael, I already did this :) See #67826 + swiftlang/swift-syntax#2041. There's one more commit on the swift-syntax side that's needed to enable testing of experimental features (swiftlang/swift-syntax@b0414b1), I can land that separately if needed.

@gottesmm
Copy link
Contributor

gottesmm commented Aug 11, 2023

@hamishknight can you land it separately in that case? My work is almost ready to go

@hamishknight
Copy link
Contributor Author

Sure thing: swiftlang/swift-syntax#2051

@hamishknight hamishknight force-pushed the then branch 3 times, most recently from f4f1802 to ebfe6aa Compare August 24, 2023 11:09
@airspeedswift
Copy link
Member

swiftlang/swift-syntax#2000

@swift-ci please build toolchain

@hamishknight hamishknight force-pushed the then branch 2 times, most recently from dfc038b to 6ca310d Compare August 31, 2023 10:12
let x = if .random() {
print("hello")
then 0
// expected-error@-1 {{cannot find 'then' in scope}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be diagnosed as a disabled feature instead if then here otherwise would be valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a source breaking change, so we can't generally diagnose it as a disabled feature, we could apply a heuristic to generate that diagnostic, but I'm not convinced it's worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not worth it if we are going to enable this by default soon, but it would be great for user experience to hint that all they need is to enable a flag in situations when it's syntactically unambiguous that they meant then statement.

These allow multi-statement `if`/`switch` expression
branches that can produce a value at the end by
saying `then <expr>`. This is gated behind
`-enable-experimental-feature ThenStatements`
pending evolution discussion.
@hamishknight
Copy link
Contributor Author

swiftlang/swift-syntax#2000

@swift-ci please test

@airspeedswift
Copy link
Member

swiftlang/swift-syntax#2000

@swift-ci please build toolchain

@hamishknight hamishknight merged commit 8dc55b8 into swiftlang:main Sep 1, 2023
@hamishknight hamishknight deleted the then branch September 1, 2023 21:32
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.

5 participants