Xcode 11.4 beta triggers false positive for implicit_getter rule

[chrisfsampaio]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

Describe the bug

Given the following code:

struct Foo {
    private var _bar: Bool
}

extension Foo {
    var bar: Bool {
        get { _bar }
        set { self._bar = newValue }
    }
}

The getter triggers the implicit_getter warning in Xcode 11.4 beta.
Note that this wasn't an issue on previous versions of Xcode and that it seems to be a bug isolated to extensions.

Environment

• SwiftLint version 0.38.2
• Installation method used: CocoaPods
• Xcode version: 11.4 beta

[rjchatfield]

May be related, but valid_ibinspectable also fails if you annotate a computed property as @IBInspectable:

extension Foo {
    @IBInspectable var bar: Bool {
        get { _bar }
        set { self._bar = newValue }
    }
}

[marcelofabri]

Swift 5.2 removed "key.accessibility" and "key.setter_accessibility" (which are used in both rules implementations) from computed variables in extensions:

Swift 5.1

{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.length" : 145,
  "key.offset" : 0,
  "key.substructure" : [
    {
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.bodylength" : 28,
      "key.bodyoffset" : 12,
      "key.kind" : "source.lang.swift.decl.struct",
      "key.length" : 41,
      "key.name" : "Foo",
      "key.namelength" : 3,
      "key.nameoffset" : 7,
      "key.offset" : 0,
      "key.substructure" : [
        {
          "key.accessibility" : "source.lang.swift.accessibility.private",
          "key.attributes" : [
            {
              "key.attribute" : "source.decl.attribute.private",
              "key.length" : 7,
              "key.offset" : 17
            }
          ],
          "key.kind" : "source.lang.swift.decl.var.instance",
          "key.length" : 14,
          "key.name" : "_bar",
          "key.namelength" : 4,
          "key.nameoffset" : 29,
          "key.offset" : 25,
          "key.setter_accessibility" : "source.lang.swift.accessibility.private",
          "key.typename" : "Bool"
        }
      ]
    },
    {
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.bodylength" : 85,
      "key.bodyoffset" : 58,
      "key.kind" : "source.lang.swift.decl.extension",
      "key.length" : 101,
      "key.name" : "Foo",
      "key.namelength" : 3,
      "key.nameoffset" : 53,
      "key.offset" : 43,
      "key.substructure" : [
        {
          "key.accessibility" : "source.lang.swift.accessibility.internal",
          "key.bodylength" : 63,
          "key.bodyoffset" : 78,
          "key.kind" : "source.lang.swift.decl.var.instance",
          "key.length" : 79,
          "key.name" : "bar",
          "key.namelength" : 3,
          "key.nameoffset" : 67,
          "key.offset" : 63,
          "key.setter_accessibility" : "source.lang.swift.accessibility.internal",
          "key.typename" : "Bool"
        }
      ]
    }
  ]
}

Swift 5.2

{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.length" : 145,
  "key.offset" : 0,
  "key.substructure" : [
    {
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.bodylength" : 28,
      "key.bodyoffset" : 12,
      "key.kind" : "source.lang.swift.decl.struct",
      "key.length" : 41,
      "key.name" : "Foo",
      "key.namelength" : 3,
      "key.nameoffset" : 7,
      "key.offset" : 0,
      "key.substructure" : [
        {
          "key.accessibility" : "source.lang.swift.accessibility.private",
          "key.attributes" : [
            {
              "key.attribute" : "source.decl.attribute.private",
              "key.length" : 7,
              "key.offset" : 17
            }
          ],
          "key.kind" : "source.lang.swift.decl.var.instance",
          "key.length" : 14,
          "key.name" : "_bar",
          "key.namelength" : 4,
          "key.nameoffset" : 29,
          "key.offset" : 25,
          "key.setter_accessibility" : "source.lang.swift.accessibility.private",
          "key.typename" : "Bool"
        }
      ]
    },
    {
      "key.bodylength" : 85,
      "key.bodyoffset" : 58,
      "key.kind" : "source.lang.swift.decl.extension",
      "key.length" : 101,
      "key.name" : "Foo",
      "key.namelength" : 3,
      "key.nameoffset" : 53,
      "key.offset" : 43,
      "key.substructure" : [
        {
          "key.bodylength" : 63,
          "key.bodyoffset" : 78,
          "key.kind" : "source.lang.swift.decl.var.instance",
          "key.length" : 79,
          "key.name" : "bar",
          "key.namelength" : 3,
          "key.nameoffset" : 67,
          "key.offset" : 63,
          "key.typename" : "Bool"
        }
      ]
    }
  ]
}

I recommend filing a bug in https://bugs.swift.org for this.

[nathawes]

@marcelofabri Their removal in this case was actually intentional. SourceKit's open and edit requests are purely syntactic and don't use the type checker for performance reasons, so they don't actually know what accessibility level extensions and their members have without explicit accessibility annotations. They would previously just report them all as 'internal' in such cases. E.g. in the code below 'x' is actually private, but SourceKit would just report it as internal because without semantic info it doesn't know that 'Foo' refers to a private class:

private class Foo {}

extension Foo {
  var x: Int { return 42 } // 'x' is private but reported as 'internal'
}

This behavior overloaded internal to mean both "it's actually internal" and "SourceKit doesn't know", so we decided to not report the accessibility level in the cases where it doesn't know and let clients decide whether they want to assume 'internal' by default or not.

If I'm understanding the "implicit_getter" rule correctly, it seems the issue here is that it isn't really looking for the accessibility level at all, but is using the presence of the key.setter_accessibility as an indication that the the variable declaration has an explicit setter, or is settable, or something like that. What information are the rules actually looking for here? Maybe it's something SourceKit can report more explicitly or is available in a different request. For example, it could check for the get/set keywords in the key.syntaxmap entries that corresponding to the variable's range, or make a cursorinfo request at the variable's position and look at the value of key.fully_annotated_decl.

As a bit of SwiftSyntax plug, using SwiftSyntax rather SourceKit for these kinds of purely-syntactic rules might be a better fit, for example:

import SwiftSyntax
import Foundation

/// Finds explicit getters with no other accessors in their parent var's accessor list.
class SoloExplicitGetChecker: SyntaxVisitor {
  var soloExplicitGets: [TokenSyntax] = []

  override func visit(_ node: AccessorDeclSyntax) -> SyntaxVisitorContinueKind {
    if node.accessorKind.tokenKind == .contextualKeyword("get") {
      if let parent = node.parent?.as(AccessorListSyntax.self), parent.count == 1 {
        soloExplicitGets.append(node.accessorKind)
      }
    }
    return .visitChildren
  }
}

// Check for solo explicit getters in a test file and report them
let testFile = "/tmp/test.swift"
let sourceTree = try! SyntaxParser.parse(URL(fileURLWithPath: testFile))
let locationConverter = SourceLocationConverter(file: testFile, tree: sourceTree)
let checker = SoloExplicitGetChecker()

checker.walk(sourceTree)

for token in checker.soloExplicitGets {
  let loc = token.startLocation(converter: locationConverter)
  print("Make getter implicit on line \(loc.line!) column \(loc.column!) of \(loc.file!)")
}
exit(checker.soloExplicitGets.isEmpty ? 0 : 1)

[marcelofabri]

Thanks for the detailed response @nathawes!

If I'm understanding the "implicit_getter" rule correctly, it seems the issue here is that it isn't really looking for the accessibility level at all, but is using the presence of the key.setter_accessibility as an indication that the the variable declaration has an explicit setter, or is settable, or something like that.

Yes, that's what it's doing - using it to infer if it's a readonly property or not. We use that assumption in a few other rules as well.

For example, it could check for the get/set keywords in the key.syntaxmap entries that corresponding to the variable's range

That's doable for this rule, but in theory you could have another type declaration inside of a computed property body, so we'd need to handle that case.

However, other rules use this to treat let/var in a different way and we'd need to change the implementation in those cases to look for those keywords.

make a cursorinfo request at the variable's position and look at the value of key.fully_annotated_decl

That only works if we provide a buildlog, right? This is how the (experimental) "analyzer" rules work right now, but they're very slow and require a clean build

As a bit of SwiftSyntax plug, using SwiftSyntax rather SourceKit for these kinds of purely-syntactic rules might be a better fit

That's my long term goal, I have a PR adding it to the project (#3054), but there're some tradeoff:

• SwiftSyntax tags require specific Swift versions to compile and we usually support compiling SwiftLint with older Swift toolchains
• The repo doesn't have a Xcode project, which means we'll have to drop Carthage support for SwiftLintFramework
• SwiftSyntax doesn't have a CocoaPods spec, so we need to either create one or not use it when someones uses SwiftLintFramework via CP

Besides that, SwiftSyntax on Swift 5.1 is still a bit slower than using SourceKit directly. I know that this is much better on the nightly builds already, but we can't rely on them until there's a new stable release. That's why I was planing to use it mainly for opt-in rules until Swift 5.2 is officially released.

[nathawes]

For example, it could check for the get/set keywords in the key.syntaxmap entries that corresponding to the variable's range

That's doable for this rule, but in theory you could have another type declaration inside of a computed property body, so we'd need to handle that case.

@marcelofabri Yeah, I suspect it's pretty uncommon, but you'd need to ignore any occurrences within the ranges of the key.substructure entries if you want to handle it. I think this is the only real way to go for rules that want to know what the user explicitly wrote, (as far as the available SourceKit requests are concerned I mean, including the old accessibility attribute behavior).

However, other rules use this to treat let/var in a different way and we'd need to change the implementation in those cases to look for those keywords.

make a cursorinfo request at the variable's position and look at the value of key.fully_annotated_decl

That only works if we provide a buildlog, right? This is how the (experimental) "analyzer" rules work right now, but they're very slow and require a clean build

You won't get types for everything without proper arguments (and valid Swift code), but if you just pass the file name as the only compiler argument you'll still get a response for every declaration location, just not references unless they're locally resolvable. For declarations, if a type couldn't be resolved you'll see "<<error type>>" wherever you'd normally get a type name in the response. The trailing comments below are what you get from the key.annotated_decl for various stored and computed var and let declarations (I left off the xml tags wrapping the type names for clarity though):

let a = 10                       // let a: Int
let b: Int = 10                  // var e: Int { get }
var c = 10                       // var c: Int
var d = funcFromOtherFile()      // var d: <<error type>>
var e: Int {                     // var e: Int { get }
  get { return 45 }
}
var f: Int = 10 {                // var f: Int { get set }
  willSet { print("about to set") }
}
var g: TypeFromOtherFile {       // var g: TypeFromOtherFile { get set }
  get { return 10 }
  set { print("setting it") }
}
private(set) var h = 10          // private(set) var h: Int { get }
private(set) var h: Int {        // private(set) var h: Int { get set }
  get { return 45 }
  set { print("setting it") }
}
extension Foo {
  private(set) var h = 10        // private(set) var h: Int { get set }
}

The braces tell you whether it's computed/stored, the get/set tells you whether its settable for computed properties, and the var/let keyword tells you whether its settable for stored properties. It would be one request per declaration unlike the structure data though, so performance probably won't be as good. I think you could get this info from the syntax map / structure data too, but it'd be a bit more complicated.

As a bit of SwiftSyntax plug, using SwiftSyntax rather SourceKit for these kinds of purely-syntactic rules might be a better fit

That's my long term goal, I have a PR adding it to the project (#3054), but there're some tradeoff:

Oh, nice! That's great to hear.

• SwiftSyntax tags require specific Swift versions to compile and we usually support compiling SwiftLint with older Swift toolchains

Yeah, we definitely know this is a pain point. We're hoping we can use SwiftPM's proposed binary dependencies feature in future to drop the requirement that the toolchain has a compatible version of the parser library.

• The repo doesn't have a Xcode project, which means we'll have to drop Carthage support for SwiftLintFramework
• SwiftSyntax doesn't have a CocoaPods spec, so we need to either create one or not use it when someones uses SwiftLintFramework via CP

I'm not that familiar with Carthage/Cocoapods, but I assume you mean they need to be in the SwiftSyntax repo? I'm not sure there'd be much objection to adding them if someone's willing to keep an eye on/maintain them.

Besides that, SwiftSyntax on Swift 5.1 is still a bit slower than using SourceKit directly. I know that this is much better on the nightly builds already, but we can't rely on them until there's a new stable release. That's why I was planing to use it mainly for opt-in rules until Swift 5.2 is officially released.

Yeah that makes sense. I was meaning SwiftSyntax as a more long term solution rather than for this bug specifically. SourceKit's syntactic requests aren't getting too much attention anymore by comparison.