Skip to content

Commit 9bb92d4

Browse files
dabelknapdabelknap
authored
Merge pull request swiftlang#209 from google/mojave-fix-linguistic-tagger
Fix brief summary sentence detection on Mojave.
2 parents e162a18 + 1f90418 commit 9bb92d4

File tree

2 files changed

+212
-63
lines changed

2 files changed

+212
-63
lines changed

‎Sources/SwiftFormatRules/BeginDocumentationCommentWithOneLineSummary.swift

Lines changed: 131 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,103 +19,188 @@ import SwiftSyntax
1919
/// Lint: If a comment does not begin with a single-line summary, a lint error is raised.
2020
///
2121
/// - SeeAlso: https://google.github.io/swift#single-sentence-summary
22-
public final class BeginDocumentationCommentWithOneLineSummary: SyntaxLintRule {
22+
public final class BeginDocumentationCommentWithOneLineSummary: SyntaxLintRule {
23+
24+
/// Unit tests can testably import this module and set this to true in order to force the rule
25+
/// to use the fallback (simple period separator) mode instead of the `NSLinguisticTag` mode,
26+
/// even on platforms that support the latter (currently only Apple OSes).
27+
///
28+
/// This allows test runs on those platforms to test both implementations.
29+
static var forcesFallbackModeForTesting = false
30+
2331
override public func visit(_ node: FunctionDeclSyntax) {
24-
diagnoseDocComments(node)
32+
diagnoseDocComments(in: node)
2533
}
2634

2735
override public func visit(_ node: EnumDeclSyntax) {
28-
diagnoseDocComments(node)
36+
diagnoseDocComments(in: node)
2937
}
3038

3139
override public func visit(_ node: InitializerDeclSyntax) {
32-
diagnoseDocComments(node)
40+
diagnoseDocComments(in: node)
3341
}
3442

3543
override public func visit(_ node: DeinitializerDeclSyntax) {
36-
diagnoseDocComments(node)
44+
diagnoseDocComments(in: node)
3745
}
3846

3947
override public func visit(_ node: SubscriptDeclSyntax) {
40-
diagnoseDocComments(node)
48+
diagnoseDocComments(in: node)
4149
}
4250

4351
override public func visit(_ node: ClassDeclSyntax) {
44-
diagnoseDocComments(node)
52+
diagnoseDocComments(in: node)
4553
}
4654

4755
override public func visit(_ node: VariableDeclSyntax) {
48-
diagnoseDocComments(node)
56+
diagnoseDocComments(in: node)
4957
}
5058

5159
override public func visit(_ node: StructDeclSyntax) {
52-
diagnoseDocComments(node)
60+
diagnoseDocComments(in: node)
5361
}
5462

5563
override public func visit(_ node: ProtocolDeclSyntax) {
56-
diagnoseDocComments(node)
64+
diagnoseDocComments(in: node)
5765
}
5866

5967
override public func visit(_ node: TypealiasDeclSyntax) {
60-
diagnoseDocComments(node)
68+
diagnoseDocComments(in: node)
6169
}
6270

63-
/// Diagnose documentation comments that don't start
64-
/// with one sentence summary.
65-
func diagnoseDocComments(_ decl: DeclSyntax) {
71+
override public func visit(_ node: AssociatedtypeDeclSyntax) {
72+
diagnoseDocComments(in: node)
73+
}
74+
75+
/// Diagnose documentation comments that don't start with one sentence summary.
76+
private func diagnoseDocComments(in decl: DeclSyntax) {
6677
guard let commentText = decl.docComment else { return }
6778
let docComments = commentText.components(separatedBy: "\n")
6879
guard let firstPart = firstParagraph(docComments) else { return }
6980

70-
let commentSentences = sentences(in: firstPart)
71-
if commentSentences.count > 1 {
72-
diagnose(.docCommentRequiresOneSentenceSummary(commentSentences.first!), on: decl)
81+
let trimmedText = firstPart.trimmingCharacters(in: .whitespacesAndNewlines)
82+
let (commentSentences, trailingText) = sentences(in: trimmedText)
83+
if commentSentences.count == 0 {
84+
diagnose(.terminateSentenceWithPeriod(trimmedText), on: decl)
85+
}
86+
else if commentSentences.count > 1 {
87+
diagnose(.addBlankLineAfterFirstSentence(commentSentences[0]), on: decl)
88+
if !trailingText.isEmpty {
89+
diagnose(.terminateSentenceWithPeriod(trailingText), on: decl)
90+
}
7391
}
7492
}
7593

7694
/// Returns the text of the first part of the comment,
77-
func firstParagraph(_ comments: [String]) -> String? {
95+
private func firstParagraph(_ comments: [String]) -> String? {
7896
var text = [String]()
7997
var index = 0
80-
while index < comments.count &&
81-
comments[index] != "*" &&
82-
comments[index] != "" {
98+
while index < comments.count && comments[index] != "*" && comments[index] != "" {
8399
text.append(comments[index])
84-
index = index + 1
100+
index += 1
85101
}
86102
return comments.isEmpty ? nil : text.joined(separator:" ")
87103
}
88104

89105
/// Returns all the sentences in the given text.
90-
func sentences(in text: String) -> [String] {
91-
var sentences = [String]()
92-
if #available(OSX 10.13, *) { /// add linux condition
93-
let tagger = NSLinguisticTagger(tagSchemes: [.tokenType], options: 0)
94-
tagger.string = text
95-
let range = NSRange(location: 0, length: text.utf16.count)
96-
let options: NSLinguisticTagger.Options = [.omitWhitespace, .omitOther]
97-
tagger.enumerateTags(
98-
in: range,
99-
unit: .sentence,
100-
scheme: .tokenType,
101-
options: options
102-
) {_, tokenRange, _ in
103-
let sentence = (text as NSString).substring(with: tokenRange)
104-
sentences.append(sentence)
106+
///
107+
/// This function uses linguistic APIs if they are available on the current platform; otherwise,
108+
/// simpler (and less accurate) character-based string APIs are substituted.
109+
///
110+
/// - Parameter text: The text from which sentences should be extracted.
111+
/// - Returns: A tuple of two values: `sentences`, the array of sentences that were found, and
112+
/// `trailingText`, which is any non-whitespace text after the last sentence that was not
113+
/// terminated by sentence terminating punctuation. Note that if the entire string is a sequence
114+
/// of words that contains _no_ terminating punctuation, the returned array will be empty to
115+
/// indicate that there were no _complete_ sentences found, and `trailingText` will contain the
116+
/// actual text).
117+
private func sentences(in text: String) -> (sentences: [String], trailingText: Substring) {
118+
#if os(macOS)
119+
if BeginDocumentationCommentWithOneLineSummary.forcesFallbackModeForTesting {
120+
return nonLinguisticSentenceApproximations(in: text)
121+
}
122+
123+
var sentences = [String]()
124+
var tokenRanges = [Range<String.Index>]()
125+
let tags = text.linguisticTags(
126+
in: text.startIndex..<text.endIndex,
127+
scheme: NSLinguisticTagScheme.lexicalClass.rawValue,
128+
tokenRanges: &tokenRanges)
129+
let sentenceTerminatorIndices =
130+
tags.enumerated().filter { $0.element == "SentenceTerminator" }
131+
.map { tokenRanges[$0.offset].lowerBound }
132+
133+
var previous = text.startIndex
134+
for index in sentenceTerminatorIndices {
135+
let sentenceRange = previous...index
136+
sentences.append(text[sentenceRange].trimmingCharacters(in: .whitespaces))
137+
previous = text.index(after: index)
105138
}
106-
} else {
107-
return text.components(separatedBy: ". ")
139+
140+
return (sentences: sentences, trailingText: text[previous..<text.endIndex])
141+
#else
142+
return nonLinguisticSentenceApproximations(in: text)
143+
#endif
144+
}
145+
146+
/// Returns the best approximation of sentences in the given text using string splitting around
147+
/// periods that are followed by spaces.
148+
///
149+
/// This method is a fallback for platforms (like Linux, currently) where `String` does not
150+
/// support `NSLinguisticTagger` and its related APIs. It will fail to catch certain kinds of
151+
/// sentences (such as those containing abbrevations that are followed by a period, like "Dr.")
152+
/// that the more advanced API can handle.
153+
private func nonLinguisticSentenceApproximations(in text: String) -> (
154+
sentences: [String], trailingText: Substring
155+
) {
156+
// If we find a period followed by a space, then there is definitely one (approximate) sentence;
157+
// there may be more.
158+
let possiblyHasMultipleSentences = text.range(of: ". ") != nil
159+
160+
// If the string does not end in a period, then the text preceding it (up until the last
161+
// sentence terminator, or the beginning of the string, whichever comes first), is trailing
162+
// text.
163+
let hasTrailingText = !text.hasSuffix(".")
164+
165+
if !possiblyHasMultipleSentences {
166+
// If we didn't find a ". " sequence, then we either have trailing text (if there is no period
167+
// at the end of the string) or we have a single sentence (if there is a final period).
168+
if hasTrailingText {
169+
return (sentences: [], trailingText: text[...])
170+
}
171+
else {
172+
return (sentences: [text], trailingText: "")
173+
}
174+
}
175+
176+
// Otherwise, split the string around ". " sequences. All of these but the last one are
177+
// definitely (approximate) sentences. The last one is either trailing text or another sentence,
178+
// depending on whether the entire string ended with a period.
179+
let splitText = text.components(separatedBy: ". ")
180+
let definiteApproximateSentences = splitText.dropLast().map { "\($0)." }
181+
let trailingText = splitText.last ?? ""
182+
if hasTrailingText {
183+
return (sentences: Array(definiteApproximateSentences), trailingText: trailingText[...])
184+
}
185+
else {
186+
var sentences = Array(definiteApproximateSentences)
187+
sentences.append(trailingText)
188+
return (sentences: sentences, trailingText: "")
108189
}
109-
return sentences
110190
}
111191
}
112192

113193
extension Diagnostic.Message {
114-
static func docCommentRequiresOneSentenceSummary(_ firstSentence: String) -> Diagnostic.Message {
115-
let sentenceWithoutExtraSpaces = firstSentence.trimmingCharacters(in: .whitespacesAndNewlines)
116-
return .init(
117-
.warning,
118-
"add a blank comment line after this sentence: \"\(sentenceWithoutExtraSpaces)\""
119-
)
194+
195+
static func terminateSentenceWithPeriod<Sentence: StringProtocol>(_ text: Sentence)
196+
-> Diagnostic.Message
197+
{
198+
return .init(.warning, "terminate this sentence with a period: \"\(text)\"")
199+
}
200+
201+
static func addBlankLineAfterFirstSentence<Sentence: StringProtocol>(_ text: Sentence)
202+
-> Diagnostic.Message
203+
{
204+
return .init(.warning, "add a blank comment line after this sentence: \"\(text)\"")
120205
}
121206
}

‎Tests/SwiftFormatRulesTests/BeginDocumentationCommentWithOneLineSummaryTests.swift

Lines changed: 81 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,17 @@ import XCTest
55
@testable import SwiftFormatRules
66

77
public class BeginDocumentationCommentWithOneLineSummaryTests: DiagnosingTestCase {
8+
9+
public override func setUp() {
10+
// Reset this to false by default. Specific tests may override it.
11+
BeginDocumentationCommentWithOneLineSummary.forcesFallbackModeForTesting = false
12+
super.setUp()
13+
}
14+
815
public func testDocLineCommentsWithoutOneSentenceSummary() {
916
let input =
1017
"""
11-
/// Returns a bottle of Dr. Pepper from the vending machine.
18+
/// Returns a bottle of Dr Pepper from the vending machine.
1219
public func drPepper(from vendingMachine: VendingMachine) -> Soda {}
1320
1421
/// Contains a comment as description that needs a sentece
@@ -32,17 +39,21 @@ public class BeginDocumentationCommentWithOneLineSummaryTests: DiagnosingTestCas
3239
3340
/// This docline should not succeed. There are two sentences.
3441
public enum Token { case comma, semicolon, identifier }
42+
43+
/// Should fail because it doesn't have a period
44+
public class testNoPeriod {}
3545
"""
3646
performLint(BeginDocumentationCommentWithOneLineSummary.self, input: input)
37-
XCTAssertDiagnosed(.docCommentRequiresOneSentenceSummary("This docline should not succeed."))
38-
XCTAssertDiagnosed(.docCommentRequiresOneSentenceSummary("This docline should not succeed."))
39-
40-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary(
41-
"Returns a bottle of Dr. Pepper from the vending machine."))
42-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary(
47+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This docline should not succeed."))
48+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This docline should not succeed."))
49+
XCTAssertDiagnosed(.terminateSentenceWithPeriod("Should fail because it doesn't have a period"))
50+
51+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence(
52+
"Returns a bottle of Dr Pepper from the vending machine."))
53+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence(
4354
"Contains a comment as description that needs a sentece of two lines of code."))
44-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary("The background color of the view."))
45-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary("Returns the sum of the numbers."))
55+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence("The background color of the view."))
56+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence("Returns the sum of the numbers."))
4657
}
4758

4859
public func testBlockLineCommentsWithoutOneSentenceSummary() {
@@ -76,21 +87,74 @@ public class BeginDocumentationCommentWithOneLineSummaryTests: DiagnosingTestCas
7687
public class TestClass {}
7788
/** This block comment should not succeed, enum. There are two sentences. */
7889
public enum testEnum {}
90+
/** Should fail because it doesn't have a period */
91+
public class testNoPeriod {}
7992
"""
8093
performLint(BeginDocumentationCommentWithOneLineSummary.self, input: input)
81-
XCTAssertDiagnosed(.docCommentRequiresOneSentenceSummary("This block comment should not succeed, struct."))
82-
XCTAssertDiagnosed(.docCommentRequiresOneSentenceSummary("This block comment should not succeed, class."))
83-
XCTAssertDiagnosed(.docCommentRequiresOneSentenceSummary("This block comment should not succeed, enum."))
84-
85-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary("Returns the numeric value."))
86-
XCTAssertNotDiagnosed(.docCommentRequiresOneSentenceSummary(
94+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This block comment should not succeed, struct."))
95+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This block comment should not succeed, class."))
96+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This block comment should not succeed, enum."))
97+
XCTAssertDiagnosed(.terminateSentenceWithPeriod("Should fail because it doesn't have a period"))
98+
99+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence("Returns the numeric value."))
100+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence(
87101
"This block comment contains a sentence summary of two lines of code."))
88102
}
89103

104+
#if os(macOS)
105+
public func testApproximationsOnMacOS() {
106+
// Let macOS also verify that the fallback mode works, which gives us signal about whether it
107+
// will also succeed on Linux (where the linguistic APIs are not currently available).
108+
BeginDocumentationCommentWithOneLineSummary.forcesFallbackModeForTesting = true
109+
110+
let input =
111+
"""
112+
/// Returns a bottle of Dr Pepper from the vending machine.
113+
public func drPepper(from vendingMachine: VendingMachine) -> Soda {}
114+
115+
/// Contains a comment as description that needs a sentece
116+
/// of two lines of code.
117+
public var twoLinesForOneSentence = "test"
118+
119+
/// The background color of the view.
120+
var backgroundColor: UIColor
121+
122+
/// Returns the sum of the numbers.
123+
///
124+
/// - Parameter numbers: The numbers to sum.
125+
/// - Returns: The sum of the numbers.
126+
func sum(_ numbers: [Int]) -> Int {
127+
// ...
128+
}
129+
130+
/// This docline should not succeed.
131+
/// There are two sentences without a blank line between them.
132+
struct Test {}
133+
134+
/// This docline should not succeed. There are two sentences.
135+
public enum Token { case comma, semicolon, identifier }
136+
137+
/// Should fail because it doesn't have a period
138+
public class testNoPeriod {}
139+
"""
140+
performLint(BeginDocumentationCommentWithOneLineSummary.self, input: input)
141+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This docline should not succeed."))
142+
XCTAssertDiagnosed(.addBlankLineAfterFirstSentence("This docline should not succeed."))
143+
XCTAssertDiagnosed(.terminateSentenceWithPeriod("Should fail because it doesn't have a period"))
144+
145+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence(
146+
"Returns a bottle of Dr Pepper from the vending machine."))
147+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence(
148+
"Contains a comment as description that needs a sentece of two lines of code."))
149+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence("The background color of the view."))
150+
XCTAssertNotDiagnosed(.addBlankLineAfterFirstSentence("Returns the sum of the numbers."))
151+
}
152+
#endif
153+
90154
#if !os(macOS)
91155
static let allTests = [
92156
BeginDocumentationCommentWithOneLineSummaryTests.testDocLineCommentsWithoutOneSentenceSummary,
93-
BeginDocumentationCommentWithOneLineSummaryTests.testBlockLineCommentsWithoutOneSentenceSummary
94-
]
157+
BeginDocumentationCommentWithOneLineSummaryTests.testBlockLineCommentsWithoutOneSentenceSummary,
158+
]
95159
#endif
96160
}

0 commit comments

Comments
 (0)