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

[DNM][SE-NNNN] Parse is case #1414

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

WowbaggersLiquidLunch
Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch commented Mar 17, 2023

A small piece of the puzzle.

This PR allows SwiftSyntax to parse is case pattern patching expressions, as pitched on the forums.

2 questions for reviewers:

  1. Currently as implemented, this PR allows is case to be used with value-binding like this:

    foo is case let .bar(baz)

    My intention is to allow value-bindings like this only inside conditions:

    guard foo is case let .bar(baz) // good, allowed
    
    let a = b is case let c         // bad, disallowed

    Is it possible to diagnose this on the syntactic level in this package? My understanding so far is that this can only be done during semantic analysis.

  2. Is this parser used along with the old one written in C++ in the compiler, or is this the only one used? Do I need to re-implement the same logic in the C++ version too?

@@ -735,7 +735,7 @@ public enum Keyword: UInt8, Hashable {
}
}

/// Whether the token kind is switched from being an identifier to a keyword in the lexer.
/// Whether the token kind is switched from being an identifier to being an identifier to a keyword in 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.

This is generated. I don't know what's causing the duplicate words here.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #1421 for this.

@WowbaggersLiquidLunch
Copy link
Contributor Author

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Mar 17, 2023

  1. Currently as implemented, this PR allows is case to be used with value-binding like this:

    foo is case let .bar(baz)

    My intention is to allow value-bindings like this only inside conditions:

    guard foo is case let .bar(baz) // good, allowed
    
    let a = b is case let c         // bad, diallowed

    Is it possible to diagnose this on the syntactic level in this package? My understanding so far is that this can only be done during semantic analysis.

I haven’t read the entire proposal pitch but allowing value bindings in is case seems a bit like overkill to me since we can already do that using if case .base(let bad) = foo, so my impression from just skimming over Jordan’s pitch was to not allow value bindings in is case so that is case always just evaluates to a boolean.

  1. Is this parser used along with the old one written in C++ in the compiler, or is this the only one used? Do I need to re-implement the same logic in the C++ version too?

The compiler still uses the C++ parser so you’ll need to implement the same in the C++ parser as well.

@WowbaggersLiquidLunch
Copy link
Contributor Author

I haven’t read the entire proposal pitch but allowing value bindings in is case seems a bit like overkill to me since we can already do that using if case .base(let bad) = foo, so my impression from just skimming over Jordan’s pitch was to not allow value bindings in is case so that is case always just evaluates to a boolean.

Yes, in Jordan's pitch the intention is not allowing value-binding in is case. But some people in the pitch thread pointed out (and mentioned in the pitch itself, though in comparison to ~= not case let) that <value> is case let/var/inout <pattern> is ergonomic, better for code completion, and potentially help with further refining matches. I agree with these points. So I'm thinking if it's not a lot extra work to support value-binding, I'll default to having it.

@WowbaggersLiquidLunch
Copy link
Contributor Author

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Mar 20, 2023

Yes, in Jordan's pitch the intention is not allowing value-binding in is case. But some people in the pitch thread pointed out (and mentioned in the pitch itself, though in comparison to ~= not case let) that is case let/var/inout is ergonomic, better for code completion, and potentially help with further refining matches. I agree with these points. So I'm thinking if it's not a lot extra work to support value-binding, I'll default to having it.

Thanks for the explanation and sorry for not reading the entire thread. I imagine this is something that needs to be discussed in the Swift Evolution proposal, so I won’t leave any comment on personal preference here.

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.

2 participants