Catch implicit positions in .strings files (#48)

* Catch implicit positions in .strings files

* Formatting

Author: stevelandeyasana

Makefile

@@ -22,7 +22,7 @@ uninstall:
 	rm -f $(INSTALL_PATH)
 
 format_code:
-	swiftformat Sources Tests
+	mint run swiftformat Sources Tests
 
 # Homebrew discourages self-submission unless the project is popular, so this is commented out for now.
 # publish: zip_binary bump_brew

Sources/LocheckCommand/main.swift

@@ -105,6 +105,7 @@ struct XCStrings: HasIgnoreWithShorthand, ParsableCommand {
                     parseAndValidateXCStrings(
                         base: try! File(path: base.argument),
                         translation: translationFile,
+                        baseLanguageName: (try! File(path: base.argument)).nameExcludingExtension,
                         translationLanguageName: translationFile.nameExcludingExtension,
                         problemReporter: problemReporter)
                 }

Sources/LocheckLogic/Problems.swift

@@ -192,6 +192,27 @@ struct StringHasMissingArguments: Problem, StringsProblem, Equatable {
     var message: String { "'\(key)' does not include argument(s) at \(args.joined(separator: ", "))" }
 }
 
+struct StringHasImplicitPosition: Problem, StringsProblem, Equatable {
+    var kindIdentifier: String { "string_has_implicit_position" }
+    var uniquifyingInformation: String { "\(language)-\(key)-\(position)" }
+    var severity: Severity { .warning }
+    let base: String?
+    let translation: String?
+    let key: String
+    let value: String
+    let position: Int
+    let language: String
+    let suggestion: String
+
+    var message: String {
+        if key == value {
+            return "Argument \(position) in '\(key)' has an implicit position. Use an explicit position for safety (\(suggestion))."
+        } else {
+            return "Argument \(position) in translation of '\(key)' ('\(value)') has an implicit position. Use an explicit position for safety (\(suggestion))."
+        }
+    }
+}
+
 struct StringArrayItemCountMismatch: Problem, StringsProblem, Equatable {
     var base: String?
     var translation: String?
@@ -219,14 +240,14 @@ struct StringsdictEntryContainsNoVariablesProblem: Problem, StringsdictProblem,
 struct StringsdictEntryHasImplicitPosition: Problem, StringsdictProblem, Equatable {
     var kindIdentifier: String { "stringsdict_entry_has_implicit_position" }
     var uniquifyingInformation: String { "\(key)-\(position)-\(permutation)" }
-    // We have code to detect this, but without a way of disabling it per-project yet, it's not reported.
     var severity: Severity { .warning }
     let key: String
     let position: Int
     let permutation: String
+    let suggestion: String
 
     var message: String {
-        "Argument \(position) in permutation '\(permutation)' of '\(key)' has an implicit position. Use an explicit position for safety."
+        "Argument \(position) in permutation '\(permutation)' of '\(key)' has an implicit position. Use an explicit position for safety (\(suggestion))."
     }
 }
 

Sources/LocheckLogic/Types/FormatArgument.swift

@@ -14,6 +14,8 @@ public struct FormatArgument: Equatable, Hashable {
     let specifier: String
     let position: Int
     let isPositionExplicit: Bool
+
+    var asExplicit: String { "%$\(position)\(specifier)" }
 }
 
 extension FormatArgument {

Sources/LocheckLogic/Types/LocalizedStringPair.swift

@@ -29,6 +29,8 @@ extension LocalizedStringPair {
         string: String,
         path: String,
         line: Int,
+        basePath: String,
+        baseLineFallback: Int = 0, // Either zero (this is a translation) or a repeat of 'line' (this is the base)
         baseStringMap: [String: FormatString]? = nil) { // only pass for translation strings
         guard
             let match = Expressions.stringPairRegex.lo_matches(in: string).first,
@@ -47,7 +49,7 @@ extension LocalizedStringPair {
         if let base = baseStringMap?[key] {
             self.base = base
         } else {
-            base = FormatString(string: key, path: path, line: line)
+            base = FormatString(string: key, path: basePath, line: baseLineFallback)
         }
         translation = FormatString(string: String(valueSequence), path: path, line: line)
     }

Sources/LocheckLogic/Types/StringsdictEntry.swift

@@ -74,7 +74,8 @@ public struct StringsdictEntry: Equatable {
                         StringsdictEntryHasImplicitPosition(
                             key: key,
                             position: arg.position,
-                            permutation: string.string),
+                            permutation: string.string,
+                            suggestion: arg.asExplicit),
                         line)
                 }
 

Sources/LocheckLogic/Validators/parseAndValidateXCStrings.swift

@@ -14,22 +14,25 @@ import Foundation
 public func parseAndValidateXCStrings(
     base: File,
     translation: File,
+    baseLanguageName: String,
     translationLanguageName: String,
     problemReporter: ProblemReporter) {
-    let collectLines = { (file: File, baseStringMap: [String: FormatString]) -> [LocalizedStringPair] in
+    let collectLines = { (file: File, isBase: Bool, baseStringMap: [String: FormatString]) -> [LocalizedStringPair] in
         file.lo_getLines(problemReporter: problemReporter)?
             .enumerated()
             .compactMap {
                 LocalizedStringPair(
                     string: $0.1,
                     path: file.path,
                     line: $0.0 + 1,
+                    basePath: base.path,
+                    baseLineFallback: isBase ? $0.0 + 1 : 0,
                     baseStringMap: baseStringMap)
             } ?? []
     }
 
     // Compare similarly-named files 1-to-1
-    let baseStrings: [LocalizedStringPair] = collectLines(base, [:])
+    let baseStrings: [LocalizedStringPair] = collectLines(base, true, [:])
     guard !baseStrings.isEmpty else { return }
 
     let baseStringMap = baseStrings.lo_makeDictionary(
@@ -42,10 +45,11 @@ public func parseAndValidateXCStrings(
                 lineNumber: value.line)
         })
 
-    let translationStrings = collectLines(translation, baseStringMap)
+    let translationStrings = collectLines(translation, false, baseStringMap)
     validateStrings(
         baseStrings: baseStrings,
         translationStrings: translationStrings,
+        baseLanguageName: baseLanguageName,
         translationLanguageName: translationLanguageName,
         problemReporter: problemReporter)
 }
@@ -57,6 +61,7 @@ public func parseAndValidateXCStrings(
 func validateStrings(
     baseStrings: [LocalizedStringPair],
     translationStrings: [LocalizedStringPair],
+    baseLanguageName: String,
     translationLanguageName: String,
     problemReporter: ProblemReporter) {
     // MARK: Ensure all base strings appear in this translation
@@ -128,7 +133,38 @@ func validateStrings(
 
         let baseArgs = translationString.base.arguments.sorted(by: { $0.position < $1.position })
 
+        if baseArgs.count > 1 {
+            for arg in baseArgs where !arg.isPositionExplicit {
+                // This might be reported multiple times, but it's deduped
+                problemReporter.report(
+                    StringHasImplicitPosition(
+                        base: translationString.base.string,
+                        translation: translationString.base.string,
+                        key: translationString.key,
+                        value: translationString.base.string,
+                        position: arg.position,
+                        language: baseLanguageName,
+                        suggestion: arg.asExplicit),
+                    path: translationString.base.path,
+                    lineNumber: translationString.base.line)
+            }
+        }
+
         for arg in translationString.translation.arguments {
+            if !arg.isPositionExplicit && translationString.translation.arguments.count > 1 {
+                problemReporter.report(
+                    StringHasImplicitPosition(
+                        base: translationString.base.string,
+                        translation: translationString.translation.string,
+                        key: translationString.key,
+                        value: translationString.translation.string,
+                        position: arg.position,
+                        language: translationLanguageName,
+                        suggestion: arg.asExplicit),
+                    path: translationString.path,
+                    lineNumber: translationString.line)
+            }
+
             guard let baseArg = baseArgs.first(where: { $0.position == arg.position }) else {
                 continue // we already logged an error for this above
             }

Sources/LocheckLogic/Validators/validateLproj.swift

@@ -23,6 +23,7 @@ public func validateLproj(base: LprojFiles, translation: LprojFiles, problemRepo
         parseAndValidateXCStrings(
             base: file,
             translation: counterpart,
+            baseLanguageName: base.name,
             translationLanguageName: translation.name,
             problemReporter: problemReporter)
     }