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

False positive with orphaned_doc_comment #4573

Closed
2 tasks done
michaelpeternell opened this issue Nov 21, 2022 · 10 comments · Fixed by #4581
Closed
2 tasks done

False positive with orphaned_doc_comment #4573

michaelpeternell opened this issue Nov 21, 2022 · 10 comments · Fixed by #4581

Comments

@michaelpeternell
Copy link

New Issue Checklist

Describe the bug

Starting with Swiftlint 0.50.0 I am getting false positives from the orphaned_doc_comment rule

The following code triggers a rule violation:

func hello() {
    /// the text
    let text = "Swift"
    print("The text is \(text)")
}

This should not trigger, because it is perfectly valid to document a local variable or local let declaration.

Environment

  • SwiftLint version 0.50.0
  • Installation method used: Homebrew
@jpsim
Copy link
Collaborator

jpsim commented Nov 21, 2022

This is by design, but perhaps that design is flawed? What does a documentation comment provide you here over a regular comment? Does Xcode even render rich quick help documentation for this and even if it does, why is that helpful when the only usage is local?

@NachoSoto
Copy link
Contributor

I just ran into the same for internal declarations:

internal class C {
  /// I want to document this method for other developers of this SDK, even if it's not exposed publically
  internal static func f() {}
}

This is still very useful, and they're visible within Xcode:
Screenshot 2022-11-21 at 10 49 31

NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Nov 21, 2022
[The new release](https://github.com/realm/SwiftLint/releases/tag/0.50.0) is much faster as [it uses](realm/SwiftLint#4216) the [new SwiftParser](https://github.com/apple/swift-syntax).

### Changes:
- Disabled `orphaned_doc_comment`: it provides warnings for non-public declarations (see realm/SwiftLint#4573 (comment)), but those are still useful for us.
- `function_body_length` has a new logic
- `large_tuple` seems to have new default thresholds, so I've specified them in `.swiftlint` to relax them. Tuples will most likely even become `Equatable` soon in Swift 6.0 so they're here to stay.
@jpsim
Copy link
Collaborator

jpsim commented Nov 21, 2022

@NachoSoto your issue is completely different and I agree a valid false positive.

@NachoSoto
Copy link
Contributor

Do you want me to open a separate ticket for it?

@michaelpeternell
Copy link
Author

I rarely use doc-comments on local variables, but in large codebases it sometimes happens. Like when the purpose of a variable is a little bit confusing, or in code that is very complex.

Also, Xcode does render it when autocompleting or when option-clicking.

If you don't agree that this is a false positive, then I would just disable the orphaned_doc_comment rule altogether for my projects. Because I don't want to change doc-comments to ordinary comments, and I also don't want to delete valid doc comments.

@awein
Copy link

awein commented Nov 22, 2022

Same here: Using doc-comments on variables is not something I use a lot but it helps to make complex code more easy to read and follow when used.
Would be nice to keep using it while keeping the (otherwise very useful) rule active.

NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Nov 22, 2022
[The new
release](https://github.com/realm/SwiftLint/releases/tag/0.50.0) is much
faster as [it uses](realm/SwiftLint#4216) the
[new SwiftParser](https://github.com/apple/swift-syntax).

### Changes:
- Disabled `orphaned_doc_comment`: it provides warnings for non-public
declarations (see
realm/SwiftLint#4573 (comment)),
but those are still useful for us.
- `function_body_length` has a new logic
- `large_tuple` seems to have new default thresholds, so I've specified
them in `.swiftlint` to relax them. Tuples will most likely even become
`Equatable` soon in Swift 6.0 so they're here to stay.
@jpsim
Copy link
Collaborator

jpsim commented Nov 23, 2022

I'll split out the local doc comment validation into a separate local_scope_doc_comment rule.

I'll also make it opt-in so it's not enabled by default.

@michaelpeternell thanks for letting me know that Xcode renders quick help for local scope docstrings, I was able to confirm that in my testing too:

image

Naturally, this doesn't work for references, only declarations.

image

jpsim added a commit that referenced this issue Nov 23, 2022
Moving the validation of doc comments in local scopes out of
`orphaned_doc_comment` and into a new opt-in `local_doc_comment` rule.

Addresses #4573.
@jpsim jpsim linked a pull request Nov 23, 2022 that will close this issue
@jpsim
Copy link
Collaborator

jpsim commented Nov 23, 2022

Splitting the rule in #4581

@jpsim
Copy link
Collaborator

jpsim commented Nov 23, 2022

@NachoSoto please file a new issue with complete repro steps. I'm not sure what you're seeing, but tests pass for me when I add your code sample to the non-triggering examples for orphaned_doc_comment.

--- a/Source/SwiftLintFramework/Rules/Lint/OrphanedDocCommentRule.swift
+++ b/Source/SwiftLintFramework/Rules/Lint/OrphanedDocCommentRule.swift
@@ -31,6 +31,12 @@ struct OrphanedDocCommentRule: SourceKitFreeRule, ConfigurationProviderRule {
             /// Look here for more info:
             /// https://github.com.
             var myGreatProperty: String!
+            """),
+            Example("""
+            internal class C {
+              /// Some internal docs.
+              internal static func f() {}
+            }
             """)
         ],
         triggeringExamples: [

jpsim added a commit that referenced this issue Nov 23, 2022
Moving the validation of doc comments in local scopes out of
`orphaned_doc_comment` and into a new opt-in `local_doc_comment` rule.

Addresses #4573.
@michaelpeternell
Copy link
Author

michaelpeternell commented Nov 28, 2022

I've just installed Swiftlint 0.50.1

Shouldn't the following example trigger the orphaned_doc_comment rule?

struct Bar {
    var foo = 2
    func sayHello() {
        if foo == 1 {
            /// foo is 1, we therefore have to print!
            print("foo is 1!!!!")
        }
    }
}

this is clearly an invalid doc-comment, because it is not attached to a declaration.

But when I enable local_doc_comment it triggers on all doc comments in local scopes, even the ones that are valid.

Here an example of what I think should trigger and should not trigger a rule violation:

struct FooBar {
    var foo = 2
    func sayHello() {
        if foo == 1 {
            /// foo is 1, we therefore have to print! (this should trigger orphaned_doc_comment)
            print("foo is 1!!!!")
        }
        /// That doesn't need any explanation (this should NOT trigger orphaned_doc_comment)
        var someVar = foo + 1
    }
}

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

Successfully merging a pull request may close this issue.

4 participants