textmate / swift.tmbundle

TextMate support for Swift
72 stars 30 forks source link

Enums with _ associated value labels are marked invalid #35

Closed u2606 closed 5 years ago

u2606 commented 5 years ago

GitHub’s syntax highlighting uses github/liguist, which in turn uses swift.tmbundle. This combination of software incorrectly considers enum cases that use _ x as an associated value label (where x can be any valid identifier) as invalid Swift code (highlighted with a red background).

_ x is valid as an enum associated value in Swift, but x y is not valid, which might be the source of the issue.

Steps to reproduce

Option A:

  1. Create a .swift file in a GitHub repo
  2. Add an enum with a case with an associated value to the file
  3. Label that associated value _ foo, where foo can be any valid identifier (see example below)

Option B:

  1. Create an issue like this one (or other Markdown document) on GitHub
  2. Embed an enum with a case with an associated value in the document surrounded by ```swift and ```
  3. Label that associated value _ foo, where foo can be any valid identifier (see example below)
enum Foo {
    case one(_ x: Int) // no error, but marked as invalid
    case two(_: Int)
    case three(x y: Int) // error: enum case cannot have keyword arguments
    case four(x: Int)
    case five(__ x: Int) // error: enum case cannot have keyword arguments
}

Note the following:

Expected results

case one(_ x: Int) should be considered valid Swift code.

Actual results

case one(_ x: Int) is marked as invalid Swift code (highlighted with a red background).

Environment

GitHub, using (presumably) the latest version of Linguist, using (presumably) a recent version of swift.tmbundle.

jtbandes commented 5 years ago

It appears you're right that the Swift compiler accepts case one(_ x: Int) so we should probably fix this in the TM grammar. But why would someone want this? Is it any different from case one(Int)? In fact, the language grammar doesn't seem to mention that the former is allowed.

Relatedly, it seems there is currently a bug/limitation where

let x: Foo = .four(x: 4)  // valid
let y: Foo = .four(4)  // invalid

but

let z = Foo.four
z(x: 4)  // invalid
z(4)  // valid

I think this might be what SE-0155 is trying to fix 🤷‍♂

u2606 commented 5 years ago

Thanks for your response!

case one(_ x: Int) is not common, but I’ve seen members of my team use that syntax because it documents the associated value’s purpose without requiring the call site to use the name.

For example, if we have an enum that represents rows in a table view:

enum Row {
    case itemCount(Int)
    case subscription(_ plansDescription: String)
}

The case name case itemCount(Int) is descriptive enough to omit an associated value label. However, case subscription(String) by itself would make it difficult to determine what the String is used for. On the other hand, case subscription(plansDescription: String) would make the call site more onerous:

let plansDescription = ... // complex, multiline logic

// `case subscription(plansDescription: String)` has duplication:
let row = Row.subscription(plansDescription: plansDescription)

// `case subscription(_ plansDescription: String)` has no duplication:
let row = Row.subscription(plansDescription)

It looks like your example with four isn’t something SE-0155 was meant to address. Based on an example in the “Proposed Solution” section, z(x: 4) would still be invalid:

let f = Expr.elet
f([], anExpr) // valid, just like `z(4)`
f(locals: [], body: anExpr) // invalid, just like `z(x: 4)`

The root of the error in the z(x: 4) example is that function variables can’t have argument labels (other than _). Your example behaves exactly the same with a function as it does with an enum case constructor:

func divide(dividend: Double, divisor: Double) -> Double {
    return dividend / divisor
}

let x2 = divide(dividend: 1.0, divisor: 2.0) // valid
let y2 = divide(1.0, 2.0) // invalid

let z2 = divide
z2(dividend: 1.0, divisor: 2.0) // error: extraneous argument labels 'dividend:divisor:' in call
z2(1.0, 2.0) // valid

If we try to give argument labels to a function variable, the compiler forces us to use _:

let a: (dividend: Double, divisor: Double) -> Double = divide // error: function types cannot have argument labels; use '_' before 'dividend'; use '_' before 'divisor'

let b: (_ dividend: Double, _ divisor: Double) -> Double = divide // valid
b(dividend: 1.0, divisor: 2.0) // error: extraneous argument labels 'dividend:divisor:' in call
b(1.0, 2.0) // valid

Ideally, the syntax highlighting would mark dividend and divisor as invalid in let a: (dividend: Double, divisor: Double) -> Double = divide, too. Of course, that’s less important than marking case one(_ x: Int) as invalid.