Improve the DontRepeatTypeInStaticProperties rule to better align with its original intent

[TTOzzi]

Resolve #854

DontRepeatTypeInStaticProperties checks whether the type is included in the static property's name using contains.
Since it triggers a diagnostic not only when the type appears as a suffix but also in other positions, I have appropriately modified the message.

[allevato]

I don't think the message is the problem with this rule; the rule implementation itself is far too broad with what it detects. For example, consider one of the examples mentioned in that PR:

remove the suffix 'Data' from the name of the variable 'postgresDataType'

Presuming Data was the type of that property, that's an absolutely fine property name that doesn't violate this rule.

I don't remember precisely why we used contains in this implementation, unless it was a lackluster attempt at also catching plural terms. But at a minimum, I think we should be doing a suffix check instead of a contains check.

If you're really interested in exploring this, we could gain more insight by looking at how the compiler itself (in ClangImporter) stems type-name suffixes off the names of imported Objective-C names, and reimplement similar logic here to detect those cases.

[TTOzzi]

I don't think the message is the problem with this rule; the rule implementation itself is far too broad with what it detects. For example, consider one of the examples mentioned in that PR:

remove the suffix 'Data' from the name of the variable 'postgresDataType'

Presuming Data was the type of that property, that's an absolutely fine property name that doesn't violate this rule.

I don't remember precisely why we used contains in this implementation, unless it was a lackluster attempt at also catching plural terms. But at a minimum, I think we should be doing a suffix check instead of a contains check.

If you're really interested in exploring this, we could gain more insight by looking at how the compiler itself (in ClangImporter) stems type-name suffixes off the names of imported Objective-C names, and reimplement similar logic here to detect those cases.

Oh, I see. Thank you for your kind advice.
I’ll refer to the ClangImporter logic and reimplement it accordingly!

[TTOzzi]

I looked into the suffix-related logic in the ClangImporter, but I couldn't find anything particularly useful to add to the DontRepeatTypeInStaticProperties rule.

• Removal of the "Notification" suffix
• Removal of the "Error" suffix
• Removal of completion-handler suffixes
• Removal of the "Ref" suffix for CoreFoundation types
• Removal of the "Protocol" suffix

Most of these cases already seem to be handled by the existing logic.

Instead, I have modified the DontRepeatTypeInStaticProperties rule to better align with its description:
"Static properties of a type that return that type will yield a lint error."

[TTOzzi]

Due to implementation limitations, the following case cannot be handled:

class Foo {
  static let defaultFoo = makeFoo()

  static func makeFoo() -> Foo {
    return Foo()
  }
}

However, aside from this, the rule now behaves in a way that aligns well with its description in most cases.

[allevato]

Thanks for the updates!

I did a little more digging and it looks like the code used by the compiler for this lives here (https://github.com/swiftlang/swift/blob/b221a92f695973f15ab26a63c2a9d7599f69435f/lib/Basic/StringExtras.cpp#L1252), and ClangImporter calls into that.

But that also looks like quite a lot of logic to bring over, and I'm not sure how much the logic would pay off compared to the effort it would take to port it over. What you've done here is already a solid improvement!

[TTOzzi]

Thanks for the updates!

I did a little more digging and it looks like the code used by the compiler for this lives here (https://github.com/swiftlang/swift/blob/b221a92f695973f15ab26a63c2a9d7599f69435f/lib/Basic/StringExtras.cpp#L1252), and ClangImporter calls into that.

But that also looks like quite a lot of logic to bring over, and I'm not sure how much the logic would pay off compared to the effort it would take to port it over. What you've done here is already a solid improvement!

Oh, I wasn’t familiar with C++ enough to have checked that file.
Although the PR has already been merged, I'll take a look, and if I find any areas for further improvement, I'll consider opening another PR.
Thank you for looking into this together 🙇