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

Swift 2.3 or later break docs-related rules like missing_docs and valid_docs #728

Closed
mattrubin opened this issue Jul 22, 2016 · 14 comments
Closed
Labels
bug Unexpected and reproducible misbehavior.

Comments

@mattrubin
Copy link
Contributor

mattrubin commented Jul 22, 2016

After converting my project to Swift 3, SwiftLint warns about missing docs for nearly every public declaration. All of these declarations do have documentation comments (///) and with missing_docs enabled there were no warnings for Swift 2.3.

The issue is reproducible with Xcode 8 beta 3, SwiftLint 0.11.1, and this project: https://github.com/mattrubin/OneTimePassword/tree/8e7ec68c03f80da6dddb41a422678f08ab7d9bdb

@jpsim jpsim added the bug Unexpected and reproducible misbehavior. label Jul 25, 2016
@jpsim
Copy link
Collaborator

jpsim commented Jul 25, 2016

Hi @mattrubin, sorry about that 😬. I labeled this as a bug, hopefully someone can look into it soon.

Related: the missing_docs rule has had many false positives, many more than I'd consider acceptable for a rule that's on by default. It might be worth making it an opt-in rule until we can make it more robust.

@mattrubin
Copy link
Contributor Author

I believe missing_docs is already an opt-in rule, so it shouldn't cause problems in the default configuration.

If I can find the time, I'll try to track down the cause of the issue myself. I'm not very familiar with SourceKit(ten), though. Any initial suggestions, or thoughts on why Swift 3 might have broken this?

@yarneo
Copy link

yarneo commented Jul 25, 2016

Having the same issue

@RafaelPlantard
Copy link

Having the same issue! @jpsim any solution?

@norio-nomura
Copy link
Collaborator

This issue is caused by change of SourceKit's behavior on Swift 3.0. jpsim/SourceKitten#269

@jpsim jpsim changed the title False positives for missing_docs with Swift 3 Swift 2.3 or later break docs-related rules like missing_docs and valid_docs Nov 25, 2016
@jpsim
Copy link
Collaborator

jpsim commented Nov 25, 2016

The SourceKit behavior we rely on for docs-related rules like missing_docs and valid_docs was removed in the versions of SourceKit that ship with Swift 2.3 and later. @keith was kind enough to file a Swift bug (SR-2487) to report this to the Swift core team to learn why it was removed and to see if it can be re-introduced.

The best way to make progress on this regression is for users affected to lend their voice and comment on SR-2487 to explain how you're affected and why it'd be useful for the feature to be added back in.

@marcelofabri
Copy link
Collaborator

Closed by @jpsim in #1284.

@SURYAKANTSHARMA
Copy link

Have same issue . Working in xcode 8.2.1
screen shot 2017-03-21 at 11 30 26 am

@Blackjacx
Copy link
Contributor

I have the same issue with the upcoming false positive warnings!

@marcelofabri
Copy link
Collaborator

This rule will be disabled when using Swift 2.3 or later on the next release, because there was a regression on SourceKit (https://bugs.swift.org/browse/SR-2487).

As @jpsim told, the best you can do is to comment on that issue (and not here).

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 10, 2017

Until the SourceKit bug is fixed, I've added a custom rule within my .swiftlint.yml file like so:

missing_docs:
    included: ".*.swift"
    regex: '\n *(?!\/\/\/)(\/\/)?[^\n\/]*\n *(?:public|open)'
    name: "Missing Docs"
    message: "Types, properties and methods with public or open access level should be documented."
    severity: warning

This might not be as good as the previous implementation, but it works for most cases for me. It is relying on the /// documentation syntax though (I'm not using the /** alternative).

@Tableau-David-Potter
Copy link
Contributor

@Dschee Nice. To help others, you need to put this rule below a custom_rule: tag, like so:

custom_rules:
  missing_docs:
    included: ".*.swift"
    # etc.

One thing this doesn't support is putting '@' modifiers on their own line before a public definition, such as @discardableResult. My regex fu is pretty weak. Would someone like to come up with an expression that supports that?

Example:

/// Remove a given key and the associated value.
///
/// - parameters:
///     - key: The key for the value to remove.
///
/// - returns: The value that was removed, or `nil` if the key is not present.
@discardableResult
public func removeValue(forKey key: String) -> Value? {
   // Code to remove the value.
}

Thanks,
David

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 14, 2017

@Tableau-David-Potter Thanks for the custom_rule pointer.

Regarding your example, I couldn't come up with a solution quickly for not marking that line. But given this situation, I came up with an example, that actually didn't work with the rule up until now. As an example, I would expect this to be marked with a missing_docs warning, too:

// some other code

@discardableResult public func removeValue(forKey key: String) -> Value? {
   // Code to remove the value.
}

But the custom rule didn't cover that case. So I've changed the regex to this:

\n *(?!\/\/\/)(\/\/)?[^\n\/]*\n *(?:@\S+ )*(?:public|open)

So, although this doesn't solve your problem yet, my updated custom rule looks like this:

custom_rules:
  missing_docs:
      included: ".*.swift"
      regex: '\n *(?!\/\/\/)(\/\/)?[^\n\/]*\n *(?:@\S+ )*(?:public|open)'
      name: "Missing Docs"
      message: "Types, properties and methods with public or open access level should be documented."
      severity: warning

Regarding your issue, if you find the missing_docs rule more important than your style guide to put those @... statements on their own line, then you could use my updated rule to cover such cases and use the in-line variant.

The alternative is of course that you make an exception by adding // swiftlint:disable:this missing_docs at the end of the line where the warnings appears if you have docs.

@Jeehut
Copy link
Collaborator

Jeehut commented Aug 25, 2017

I've again tried to update the regex without success. I'm not sure if it's even possible. Here's how I deal with such situations now though – just placing the @disardableResult before the docs:

@discardableResult
/// Remove a given key and the associated value.
///
/// - parameters:
///     - key: The key for the value to remove.
///
/// - returns: The value that was removed, or `nil` if the key is not present.
public func removeValue(forKey key: String) -> Value? {
   // Code to remove the value.
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected and reproducible misbehavior.
Projects
None yet
Development

No branches or pull requests

10 participants