Add "Missing Docs" rule

[jpsim]

This is probably only useful for frameworks, and even then, I wouldn't want to turn this on for every project. So I wouldn't want to merge this in until we have opt-in rules.

[segiddins]

Since the testable attribute exists, I don't see any reason why apps would actually need to have public members at all, so unless I'm missing something, this should be OK to have enabled by default?

[jpsim]

Yeah, maybe it makes sense to enable by default.

I just noticed though that this rule is leaving out a bunch of violations:

So I'll have to figure out why that is before we can merge this.

[keith]

Just a few things from our implementation of this:

• We force docstrings on both public and internal members, which I think is nice.
• Sourcekit takes any comment above a definition and associates them. So this won't produce an error:

// MARK: - Some stuff

public func foo() {}

• Handling protocols is a pain, since there should be no warning in this case:

public protocol Foo {
    /// Something documenting this function
    public func bar()
}

public struct Baz: Foo {
    public func bar() {}
}

[jpsim]

We force docstrings on both public and internal members, which I think is nice.

This rule could be parameterized on the ACL:

missing_docs: internal

Sourcekit takes any comment above a definition and associates them. So this won't produce an error:

Yeah, I noticed that while developing this. Instead of checking for the __raw_doc_comment attribute, we should get the line number of the declaration and check to see if there's a SyntaxKind.DocComment token on the line immediately above it.

Handling protocols is a pain, since there should be no warning in this case:

I don't know what to tell you about this one. It's challenging to support your use case. The structure contains key.inheritedtypes but there's no clear way to determine which declarations are inherited.

$ sourcekitten structure --text "struct Yolo: CustomStringConvertible { var description: String { return "yolo" } }"

{
  "key.substructure" : [
    {
      "key.kind" : "source.lang.swift.decl.struct",
      "key.offset" : 0,
      "key.nameoffset" : 7,
      "key.namelength" : 4,
      "key.inheritedtypes" : [
        {
          "key.name" : "CustomStringConvertible"
        }
      ],
      "key.bodylength" : 41,
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.substructure" : [
        {
          "key.kind" : "source.lang.swift.decl.var.instance",
          "key.offset" : 39,
          "key.nameoffset" : 43,
          "key.namelength" : 11,
          "key.bodyoffset" : 64,
          "key.bodylength" : 13,
          "key.accessibility" : "source.lang.swift.accessibility.internal",
          "key.length" : 23,
          "key.typename" : "String",
          "key.name" : "description"
        }
      ],
      "key.name" : "Yolo",
      "key.elements" : [
        {
          "key.kind" : "source.lang.swift.structure.elem.typeref",
          "key.offset" : 13,
          "key.length" : 23
        }
      ],
      "key.length" : 80,
      "key.bodyoffset" : 38
    }
  ],
  "key.offset" : 0,
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.length" : 80
}

[keith]

We have solved that on ours by caching the list of protocols, and checking the uncommented members against the members of the inheritedtypes.

[keith]

Oh another problem we found with protocol extensions:

public protocol Foo {
    /**
     Some Comment
     */
    static func bar() -> Self?
}

public extension Foo {
    public static func bar() -> Self? {
        return nil
    }
}

This is interpreted as a normal extension, causing more unexpected errors.

[jpsim]

I've made progress on this rule:

I just noticed though that this rule is leaving out a bunch of violations

Turns out that Xcode just stops showing certain warnings in the UI after a certain number of them. These warnings were being generated correctly.

Sourcekit takes any comment above a definition and associates them. So this won't produce an error

I fixed this in SourceKitten, which this branch is now targeting: jpsim/SourceKitten#104.

Next, I'll parameterize this rule on the ACL so you can enforce doc comments on a specified minimum ACL or higher if you so choose.

[jpsim]

@keith this rule now skips inherited members when checking for missing docs, but only when declared in a file that's being linted. Do you think this limitation should prevent us from enabling this rule by default?

[keith]

Let me run this against our project and see how many false positives we get

[jpsim]

You'll get missing doc violations for everything you're inheriting from that's declared outside what you're linting (so Swift stdlib, Apple frameworks & 3rd party dependencies).

[keith]

Yep. Related to that this means you'll get warnings for things like this:

public class PercentDrivenInteractiveTransition: UIPercentDrivenInteractiveTransition {
    public override func startInteractiveTransition(transitionContext: UIViewControllerContextTransitioning)

We solve this simply by not requiring doc comments whenever the member has the override attribute.

[jpsim]

Actually, it'd be a good idea to skip the check if a method is override. I'll add that in.

[keith]

On one of our frameworks running this the first time resulted in 204 new warnings. We'll see how it is after the override fix.

[jpsim]

On one of our frameworks running this the first time resulted in 204 new warnings

Out of how many declarations? That number doesn't mean much on its own.

[keith]

How should I come up with this number? Just mentioning this number because with our (much nastier) rule, we have 0 warnings.

[jpsim]

How should I come up with this number?

Read the last statement printed here:

--- a/Source/SwiftLintFramework/Rules/MissingDocsRule.swift
+++ b/Source/SwiftLintFramework/Rules/MissingDocsRule.swift
@@ -9,6 +9,9 @@
 import SourceKittenFramework
 import SwiftXPC

+var documentedDeclarations = 0
+var undocumentedDeclarations = 0
+
 private func dictArrayForDictionary(dictionary: XPCDictionary, key: String) -> [[String: String]]? {
     return (dictionary[key] as? XPCArray)?.flatMap {
         ($0 as? XPCDictionary) as? [String: String]
@@ -47,8 +50,10 @@ extension File {
                 return substructureOffsets
         }
         if getDocumentationCommentBody(dictionary, syntaxMap: syntaxMap) != nil {
+            documentedDeclarations++
             return substructureOffsets
         }
+        undocumentedDeclarations++
         return substructureOffsets + [Int(offset)]
     }
 }
@@ -123,6 +128,12 @@ public struct MissingDocsRule: ParameterizedRule {
     )

     public func validateFile(file: File) -> [StyleViolation] {
+        defer {
+            print(
+                "declaration documentation ratio: " +
+                "\(documentedDeclarations)/\(undocumentedDeclarations + documentedDeclarations)"
+            )
+        }
         let acl = parameters.map { $0.value }
         return file.missingDocOffsets(file.structure.dictionary, acl: acl).map {
             StyleViolation(ruleDescription: self.dynamicType.description,

For SwiftLint, this is

declaration documentation ratio: 4/153

(Yeah, SwiftLint is terribly documented at the moment)

[jpsim]

Ok, now this skips overridden declarations.

[keith]

403/518. Now we're down to 115 with the override fix.

[jpsim]

Cool. We could also resolve protocols in Apple's frameworks to help out here. Since we know where these are, this should be relatively easy to do.

However! Xcode doesn't even show inherited docs:

So is that really a usage pattern we should be encouraging with SwiftLint? The end result is really that the declaration lacks documentation as far as the user is concerned.

[keith]

Yep. I was hoping they would fix the inherited docs stuff so that this would "just work"

[jpsim]

I've just revived this now that we support opt-in rules. I've marked this rule as opt-in because it triggers violations for declarations that aren't directly documented, but are externally.

One problem with this is since merging #314, the parameterization approach taken here doesn't work and I haven't been able to update it because Xcode is a pile of garbage and keeps crashing when I try 😬.