From 74ea7a315e3975176b0d49e8fecdd34fa7b7db4b Mon Sep 17 00:00:00 2001 From: James Fletcher Date: Sat, 29 Mar 2025 11:32:40 +1100 Subject: [PATCH 1/3] Fix incorrect error message for allRemainingInput options without values Issue #434 reported that when a label, but no values, are provided for an array @Option property, the error message incorrectly signifies a missing option instead of a missing value. PR #435 fixed this for .upToNextOption arrays, but the bug remained for .allRemainingInput arrays. In addition to tests for verifying the error message, this also adds tests to verify that an attached value counts for the validation. This was a regression for .upToNextOption introduced in #435 and fixed in #610. --- .../ArgumentParser/Parsing/ArgumentSet.swift | 17 ++++++++++ .../Parsing/SplitArguments.swift | 16 ++++++++++ .../RepeatingEndToEndTests.swift | 21 +++++++++++++ .../ErrorMessageTests.swift | 25 +++++++++++++++ .../SplitArgumentTests.swift | 31 +++++++++++++++++++ 5 files changed, 110 insertions(+) diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift index c1ebb4e5c..9626776b2 100644 --- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift +++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift @@ -395,10 +395,12 @@ struct LenientParser { try argument.initial(origin, &result) // Use an attached value if it exists... + var foundAttachedValue = false if let value = parsed.value { // This was `--foo=bar` style: try update(origin, parsed.name, value, &result) usedOrigins.formUnion(origin) + foundAttachedValue = true } else if argument.allowsJoinedValue, let (origin2, value) = inputArguments.extractJoinedElement( at: originElement) @@ -408,6 +410,21 @@ struct LenientParser { try update(origins, parsed.name, String(value), &result) usedOrigins.formUnion(origins) inputArguments.removeAll(in: usedOrigins) + foundAttachedValue = true + } + + // Fix incorrect error message + // for @Option array without values + guard + inputArguments.peekNextElement(after: originElement) != nil + else { + // No independent values to be found, which is an error if there was + // no `--foo=bar`-style value already found. + if foundAttachedValue { + break + } else { + throw errorForMissingValue(originElement, parsed) + } } // ...and then consume the rest of the arguments diff --git a/Sources/ArgumentParser/Parsing/SplitArguments.swift b/Sources/ArgumentParser/Parsing/SplitArguments.swift index bb62b574c..5e5917c0d 100644 --- a/Sources/ArgumentParser/Parsing/SplitArguments.swift +++ b/Sources/ArgumentParser/Parsing/SplitArguments.swift @@ -489,6 +489,22 @@ extension SplitArguments { return (e.index, e.value.valueString!) } + func peekNextElement(after origin: InputOrigin.Element) -> ( + InputOrigin.Element, Element + )? { + guard let start = position(after: origin) else { return nil } + // Elements are sorted by their `InputIndex`. Find the first `InputIndex` + // after `origin`: + guard + let elementIndex = elements[start...].firstIndex(where: { + $0.index.subIndex == .complete + }) + else { return nil } + + let matchedArgument = elements[elementIndex] + return (.argumentIndex(matchedArgument.index), matchedArgument) + } + /// Removes the first element in `elements`. mutating func removeFirst() { firstUnused += 1 diff --git a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift index d0b54e56c..811565269 100644 --- a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift +++ b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift @@ -132,6 +132,27 @@ extension RepeatingEndToEndTests { #expect(baz.names == ["one", "two", "--verbose", "--other", "three"]) } } + + func testParsing_repeatingStringRemaining_9() { + AssertParse(Baz.self, ["--verbose", "--names=one"]) { baz in + XCTAssertTrue(baz.verbose) + XCTAssertEqual(baz.names, ["one"]) + } + } + + func testParsing_repeatingStringRemaining_10() { + AssertParse(Baz.self, ["--verbose", "--names=one", "two"]) { baz in + XCTAssertTrue(baz.verbose) + XCTAssertEqual(baz.names, ["one", "two"]) + } + } + + func testParsing_repeatingStringRemaining_11() { + AssertParse(Baz.self, ["--names=one", "--verbose", "two"]) { baz in + XCTAssertFalse(baz.verbose) + XCTAssertEqual(baz.names, ["one", "--verbose", "two"]) + } + } } // MARK: - diff --git a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift index d2d9a29a2..f57c30a8a 100644 --- a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift +++ b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift @@ -366,6 +366,14 @@ private struct EmptyArray: ParsableArguments { var verbose = false } +private struct EmptyArrayAllRemaining: ParsableArguments { + @Option(parsing: .remaining) + var array: [String] = [] + + @Flag(name: [.short, .long]) + var verbose = false +} + extension ErrorMessageTests { static let emptyArrayCases: [ErrorCase] = [ ErrorCase( @@ -394,6 +402,23 @@ extension ErrorMessageTests { func emptyArray(_ c: ErrorCase) { expectErrorMessage(EmptyArray.self, c.arguments, c.expected) } + + func testEmptyArrayOptionAllRemaining() { + AssertErrorMessage( + EmptyArrayAllRemaining.self, ["--array"], + "Missing value for '--array '" + ) + + AssertErrorMessage( + EmptyArrayAllRemaining.self, ["--verbose", "--array"], + "Missing value for '--array '" + ) + + AssertErrorMessage( + EmptyArrayAllRemaining.self, ["-v", "--array"], + "Missing value for '--array '" + ) + } } // MARK: - diff --git a/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift b/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift index 6bfb25117..ec224847b 100644 --- a/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift +++ b/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift @@ -734,6 +734,37 @@ extension SplitArgumentTests { ) #expect(valueB.1 == "bar") } + + func testPeekNextElement() throws { + var sut = try SplitArguments(arguments: ["--foo", "bar", "-baz", "qux"]) + + // To test peekNextElement(after:) we need an index... + let (startIndex, _) = try XCTUnwrap(sut.popNext()) + + let (indexA, valueA) = try XCTUnwrap(sut.peekNextElement(after: startIndex)) + XCTAssertEqual( + indexA, + .argumentIndex(SplitArguments.Index(inputIndex: 1, subIndex: .complete)) + ) + XCTAssertEqual(valueA.value.valueString, "bar") + + let (indexB, valueB) = try XCTUnwrap(sut.peekNextElement(after: indexA)) + XCTAssertEqual( + indexB, + .argumentIndex(SplitArguments.Index(inputIndex: 2, subIndex: .complete)) + ) + XCTAssertEqual(valueB.value, .option(.name(.longWithSingleDash("baz")))) + + let (indexC, valueC) = try XCTUnwrap(sut.peekNextElement(after: indexB)) + XCTAssertEqual( + indexC, + .argumentIndex(SplitArguments.Index(inputIndex: 3, subIndex: .complete)) + ) + XCTAssertEqual(valueC.value.valueString, "qux") + + // There are no elements left. + XCTAssertNil(sut.peekNextElement(after: indexC)) + } } // MARK: - Long option with empty value (issue #958) From e6b9ac24636506fbf77b2bf3a363b2f49b064dde Mon Sep 17 00:00:00 2001 From: James Fletcher Date: Sun, 18 Oct 2026 21:42:24 +1100 Subject: [PATCH 2/3] Update tests to use Swift Testing The previous commit was authored prior to the Swift Testing migration --- .../RepeatingEndToEndTests.swift | 24 +++++------ .../ErrorMessageTests.swift | 33 ++++++++------- .../SplitArgumentTests.swift | 40 ++++++++++--------- 3 files changed, 52 insertions(+), 45 deletions(-) diff --git a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift index 811565269..3c86015ae 100644 --- a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift +++ b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift @@ -133,24 +133,24 @@ extension RepeatingEndToEndTests { } } - func testParsing_repeatingStringRemaining_9() { - AssertParse(Baz.self, ["--verbose", "--names=one"]) { baz in - XCTAssertTrue(baz.verbose) - XCTAssertEqual(baz.names, ["one"]) + @Test func parsing_repeatingStringRemaining_9() { + expectParse(Baz.self, ["--verbose", "--names=one"]) { baz in + #expect(baz.verbose) + #expect(baz.names == ["one"]) } } - func testParsing_repeatingStringRemaining_10() { - AssertParse(Baz.self, ["--verbose", "--names=one", "two"]) { baz in - XCTAssertTrue(baz.verbose) - XCTAssertEqual(baz.names, ["one", "two"]) + @Test func parsing_repeatingStringRemaining_10() { + expectParse(Baz.self, ["--verbose", "--names=one", "two"]) { baz in + #expect(baz.verbose) + #expect(baz.names == ["one", "two"]) } } - func testParsing_repeatingStringRemaining_11() { - AssertParse(Baz.self, ["--names=one", "--verbose", "two"]) { baz in - XCTAssertFalse(baz.verbose) - XCTAssertEqual(baz.names, ["one", "--verbose", "two"]) + @Test func parsing_repeatingStringRemaining_11() { + expectParse(Baz.self, ["--names=one", "--verbose", "two"]) { baz in + #expect(!baz.verbose) + #expect(baz.names == ["one", "--verbose", "two"]) } } } diff --git a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift index f57c30a8a..fbde07c69 100644 --- a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift +++ b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift @@ -398,26 +398,29 @@ extension ErrorMessageTests { expected: "Missing value for '--array '"), ] + static let emptyArrayCasesAllRemaining: [ErrorCase] = [ + ErrorCase( + id: "--array alone", + arguments: ["--array"], + expected: "Missing value for '--array '"), + ErrorCase( + id: "--verbose followed by --array", + arguments: ["--verbose", "--array"], + expected: "Missing value for '--array '"), + ErrorCase( + id: "-v followed by --array", + arguments: ["-v", "--array"], + expected: "Missing value for '--array '"), + ] + @Test(arguments: emptyArrayCases) func emptyArray(_ c: ErrorCase) { expectErrorMessage(EmptyArray.self, c.arguments, c.expected) } - func testEmptyArrayOptionAllRemaining() { - AssertErrorMessage( - EmptyArrayAllRemaining.self, ["--array"], - "Missing value for '--array '" - ) - - AssertErrorMessage( - EmptyArrayAllRemaining.self, ["--verbose", "--array"], - "Missing value for '--array '" - ) - - AssertErrorMessage( - EmptyArrayAllRemaining.self, ["-v", "--array"], - "Missing value for '--array '" - ) + @Test(arguments: emptyArrayCasesAllRemaining) + func emptyArrayAllRemaining(_ c: ErrorCase) { + expectErrorMessage(EmptyArrayAllRemaining.self, c.arguments, c.expected) } } diff --git a/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift b/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift index ec224847b..1d30f3901 100644 --- a/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift +++ b/Tests/ArgumentParserUnitTests/SplitArgumentTests.swift @@ -735,35 +735,39 @@ extension SplitArgumentTests { #expect(valueB.1 == "bar") } - func testPeekNextElement() throws { + @Test func peekNextElement() throws { var sut = try SplitArguments(arguments: ["--foo", "bar", "-baz", "qux"]) // To test peekNextElement(after:) we need an index... - let (startIndex, _) = try XCTUnwrap(sut.popNext()) + let optStartIndex = sut.popNext() + let (startIndex, _) = try #require(optStartIndex) - let (indexA, valueA) = try XCTUnwrap(sut.peekNextElement(after: startIndex)) - XCTAssertEqual( - indexA, - .argumentIndex(SplitArguments.Index(inputIndex: 1, subIndex: .complete)) + let (indexA, valueA) = try #require(sut.peekNextElement(after: startIndex)) + #expect( + indexA + == .argumentIndex( + SplitArguments.Index(inputIndex: 1, subIndex: .complete)) ) - XCTAssertEqual(valueA.value.valueString, "bar") + #expect(valueA.value.valueString == "bar") - let (indexB, valueB) = try XCTUnwrap(sut.peekNextElement(after: indexA)) - XCTAssertEqual( - indexB, - .argumentIndex(SplitArguments.Index(inputIndex: 2, subIndex: .complete)) + let (indexB, valueB) = try #require(sut.peekNextElement(after: indexA)) + #expect( + indexB + == .argumentIndex( + SplitArguments.Index(inputIndex: 2, subIndex: .complete)) ) - XCTAssertEqual(valueB.value, .option(.name(.longWithSingleDash("baz")))) + #expect(valueB.value == .option(.name(.longWithSingleDash("baz")))) - let (indexC, valueC) = try XCTUnwrap(sut.peekNextElement(after: indexB)) - XCTAssertEqual( - indexC, - .argumentIndex(SplitArguments.Index(inputIndex: 3, subIndex: .complete)) + let (indexC, valueC) = try #require(sut.peekNextElement(after: indexB)) + #expect( + indexC + == .argumentIndex( + SplitArguments.Index(inputIndex: 3, subIndex: .complete)) ) - XCTAssertEqual(valueC.value.valueString, "qux") + #expect(valueC.value.valueString == "qux") // There are no elements left. - XCTAssertNil(sut.peekNextElement(after: indexC)) + #expect(sut.peekNextElement(after: indexC) == nil) } } From a51ecc105199f5b98828f5211034faba2f1fe234 Mon Sep 17 00:00:00 2001 From: James Fletcher Date: Mon, 9 Nov 2026 10:57:48 +1100 Subject: [PATCH 3/3] Change purpose of `parsing_repeatingStringRemaining_2` test This test originally expected the `.allRemainingInput` option to use the default value when specifying the option label without a value, but this is now an error instead. The test has been updated to expect the error. --- .../ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift index 3c86015ae..df7ebb488 100644 --- a/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift +++ b/Tests/ArgumentParserEndToEndTests/RepeatingEndToEndTests.swift @@ -81,9 +81,8 @@ extension RepeatingEndToEndTests { } @Test func parsing_repeatingStringRemaining_2() { - expectParse(Baz.self, ["--names"]) { baz in - #expect(!baz.verbose) - #expect(baz.names.isEmpty) + #expect(throws: (any Error).self) { + try Baz.parse(["--names"]) } }