swift-syntax icon indicating copy to clipboard operation
swift-syntax copied to clipboard

[DNM][SE-NNNN] Parse `is case`

Open WowbaggersLiquidLunch opened this issue 2 years ago • 5 comments

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?

WowbaggersLiquidLunch avatar Mar 17 '23 00:03 WowbaggersLiquidLunch

@swift-ci please test

WowbaggersLiquidLunch avatar Mar 17 '23 01:03 WowbaggersLiquidLunch

  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.

ahoppen avatar Mar 17 '23 01:03 ahoppen

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 avatar Mar 18 '23 21:03 WowbaggersLiquidLunch

@swift-ci please test

WowbaggersLiquidLunch avatar Mar 20 '23 05:03 WowbaggersLiquidLunch

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.

ahoppen avatar Mar 20 '23 18:03 ahoppen