Fix parser handling of positional args

- Change to have better support for args like:

    "--arg1 a --arg2 b"
    "--arg1 a --arg2 b c"
    "--arg1 a c --arg2 b c"
    "c --arg1 a --arg2 b"

  where option can have default values and position of
  positional args doesn't matter that much.
- Make parser to be aware of if it's handling last option
  so that we can differentiate if error can be given i.e.
  with too many args, etc.
- Fixes #795

spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java

@@ -149,6 +149,8 @@ public interface Parser {
 		private List<OptionNode> invalidOptionNodes = new ArrayList<>();
 		private List<ArgumentResult> argumentResults = new ArrayList<>();
 		private int commandArgumentPos = 0;
+		private int optionPos = -1;
+		private long expectedOptionCount;
 
 		DefaultNodeVisitor(CommandModel commandModel, ConversionService conversionService, ParserConfig config) {
 			this.commandModel = commandModel;
@@ -166,31 +168,27 @@ public interface Parser {
 				messageResults.addAll(commonMessageResults);
 				messageResults.addAll(validateOptionIsValid(registration));
 
-
-				// add options with default values
-				Set<CommandOption> resolvedOptions1 = optionResults.stream()
+				// we should already have options defined with arguments.
+				// go through positional arguments and fill using those and
+				// then fill from option default values.
+				Set<CommandOption> resolvedOptions = optionResults.stream()
 					.map(or -> or.option())
 					.collect(Collectors.toSet());
-				registration.getOptions().stream()
-					.filter(o -> o.getDefaultValue() != null)
-					.filter(o -> !resolvedOptions1.contains(o))
-					.forEach(o -> {
-						resolvedOptions1.add(o);
-						Object value = convertOptionType(o, o.getDefaultValue());
-						optionResults.add(OptionResult.of(o, value));
-					});
 
+				// get sorted list by position as we later match by order
 				List<CommandOption> optionsForArguments = registration.getOptions().stream()
-					.filter(o -> !resolvedOptions1.contains(o))
+					.filter(o -> !resolvedOptions.contains(o))
 					.filter(o -> o.getPosition() > -1)
 					.sorted(Comparator.comparingInt(o -> o.getPosition()))
 					.collect(Collectors.toList());
 
+				// leftover arguments to match into needed options
 				List<String> argumentValues = argumentResults.stream()
 					.sorted(Comparator.comparingInt(ar -> ar.position()))
 					.map(ar -> ar.value())
 					.collect(Collectors.toList());
 
+				// try to find matching arguments
 				int i = 0;
 				for (CommandOption o : optionsForArguments) {
 					int aMax = o.getArityMax();
@@ -202,11 +200,17 @@ public interface Parser {
 
 					List<String> asdf = argumentValues.subList(i, j);
 					if (asdf.isEmpty()) {
-						optionResults.add(OptionResult.of(o, null));
+						// don't arguments so only add if we know
+						// it's going to get added later via default value
+						if (o.getDefaultValue() == null) {
+							resolvedOptions.add(o);
+							optionResults.add(OptionResult.of(o, null));
+						}
 					}
 					else {
 						Object toConvertValue = asdf.size() == 1 ? asdf.get(0) : asdf;
 						Object value = convertOptionType(o, toConvertValue);
+						resolvedOptions.add(o);
 						optionResults.add(OptionResult.of(o, value));
 					}
 
@@ -216,6 +220,16 @@ public interface Parser {
 					i = j;
 				}
 
+				// possibly fill in from default values
+				registration.getOptions().stream()
+					.filter(o -> o.getDefaultValue() != null)
+					.filter(o -> !resolvedOptions.contains(o))
+					.forEach(o -> {
+						resolvedOptions.add(o);
+						Object value = convertOptionType(o, o.getDefaultValue());
+						optionResults.add(OptionResult.of(o, value));
+					});
+
 				// can only validate after optionResults has been populated
 				messageResults.addAll(validateOptionNotMissing(registration));
 			}
@@ -234,6 +248,7 @@ public interface Parser {
 
 		@Override
 		protected void onEnterRootCommandNode(CommandNode node) {
+			expectedOptionCount = optionCountInCommand(node);
 			resolvedCommmand.add(node.getCommand());
 		}
 
@@ -243,6 +258,7 @@ public interface Parser {
 
 		@Override
 		protected void onEnterCommandNode(CommandNode node) {
+			expectedOptionCount = optionCountInCommand(node);
 			resolvedCommmand.add(node.getCommand());
 		}
 
@@ -254,6 +270,7 @@ public interface Parser {
 
 		@Override
 		protected void onEnterOptionNode(OptionNode node) {
+			optionPos++;
 			commandArgumentPos = 0;
 			currentOptions.clear();
 			currentOptionArgument.clear();
@@ -307,16 +324,40 @@ public interface Parser {
 					int max = currentOption.getArityMax() > 0 ? currentOption.getArityMax() : Integer.MAX_VALUE;
 					max = Math.min(max, currentOptionArgument.size());
 					List<String> toUse = currentOptionArgument.subList(0, max);
+					List<String> toUnused = currentOptionArgument.subList(max, currentOptionArgument.size());
+					toUnused.forEach(a -> {
+						argumentResults.add(ArgumentResult.of(a, commandArgumentPos++));
+					});
 
-					if (currentOption.getArityMin() > -1 && currentOptionArgument.size() < currentOption.getArityMin()) {
-						String arg = currentOption.getLongNames()[0];
-						commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg,
-								currentOptionArgument.size()));
+					// if we're not in a last option
+					// "--arg1 a b --arg2 c" vs "--arg1 a --arg2 b c"
+					// we know to impose restriction to argument count,
+					// last option is different as we can't really fail
+					// because number of argument to eat dependes on arity
+					// and rest would go back to positional args.
+					if (optionPos + 1 < expectedOptionCount) {
+						if (currentOption.getArityMin() > -1 && currentOptionArgument.size() < currentOption.getArityMin()) {
+							String arg = currentOption.getLongNames()[0];
+							commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg,
+									currentOptionArgument.size()));
+						}
+						else if (currentOption.getArityMax() > -1 && currentOptionArgument.size() > currentOption.getArityMax()) {
+							String arg = currentOption.getLongNames()[0];
+							commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg,
+									currentOption.getArityMax()));
+						}
 					}
-					else if (currentOption.getArityMax() > -1 && currentOptionArgument.size() > currentOption.getArityMax()) {
-						String arg = currentOption.getLongNames()[0];
-						commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg,
-								currentOption.getArityMax()));
+					else {
+						if (currentOption.getArityMin() > -1 && toUse.size() < currentOption.getArityMin()) {
+							String arg = currentOption.getLongNames()[0];
+							commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg,
+									toUse.size()));
+						}
+						else if (currentOption.getArityMax() > -1 && toUse.size() > currentOption.getArityMax()) {
+							String arg = currentOption.getLongNames()[0];
+							commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg,
+									currentOption.getArityMax()));
+						}
 					}
 
 					Object value = null;
@@ -415,5 +456,12 @@ public interface Parser {
 				})
 				.collect(Collectors.toList());
 		}
+
+		private static long optionCountInCommand(CommandNode node) {
+			if (node == null) {
+				return 0;
+			}
+			return node.getChildren().stream().filter(n -> n instanceof OptionNode).count();
+		}
 	}
 }

spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java

@@ -23,6 +23,7 @@ import java.util.Map;
 import org.junit.jupiter.api.BeforeEach;
 
 import org.springframework.shell.command.CommandRegistration;
+import org.springframework.shell.command.CommandRegistration.OptionArity;
 import org.springframework.shell.command.parser.Ast.AstResult;
 import org.springframework.shell.command.parser.Lexer.LexerResult;
 import org.springframework.shell.command.parser.Parser.ParseResult;
@@ -251,6 +252,67 @@ abstract class AbstractParsingTests {
 			.and()
 		.build();
 
+	static final CommandRegistration ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT = CommandRegistration.builder()
+		.command("root7")
+		.withOption()
+			.longNames("arg1")
+			.defaultValue("arg1default")
+			.type(String.class)
+			.position(0)
+			.and()
+		.withTarget()
+			.consumer(ctx -> {})
+			.and()
+		.build();
+
+	static final CommandRegistration ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT = CommandRegistration.builder()
+		.command("root7")
+		.withOption()
+			.longNames("arg1")
+			.defaultValue("arg1default")
+			.type(String.class)
+			.arity(OptionArity.EXACTLY_ONE)
+			.position(0)
+			.and()
+		.withOption()
+			.longNames("arg2")
+			.defaultValue("arg2default")
+			.type(String.class)
+			.arity(OptionArity.EXACTLY_ONE)
+			.position(1)
+			.and()
+		.withTarget()
+			.consumer(ctx -> {})
+			.and()
+		.build();
+
+	static final CommandRegistration ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT = CommandRegistration.builder()
+		.command("root7")
+		.withOption()
+			.longNames("arg1")
+			.defaultValue("arg1default")
+			.type(String.class)
+			.arity(OptionArity.EXACTLY_ONE)
+			.position(0)
+			.and()
+		.withOption()
+			.longNames("arg2")
+			.defaultValue("arg2default")
+			.type(String.class)
+			.arity(OptionArity.EXACTLY_ONE)
+			.position(1)
+			.and()
+		.withOption()
+			.longNames("arg3")
+			.type(String.class)
+			.arity(OptionArity.EXACTLY_ONE)
+			.position(2)
+			.and()
+		.withTarget()
+			.consumer(ctx -> {})
+			.and()
+		.build();
+
 	Map<String, CommandRegistration> registrations = new HashMap<>();
 
 	@BeforeEach

spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java

@@ -445,6 +445,195 @@ class ParserTests extends AbstractParsingTests {
 				}
 			);
 		}
-	}
 
+		@Test
+		void noArgsGetsOnlyDefault() {
+			register(ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).isEmpty();
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("arg1default");
+				}
+			);
+		}
+
+		@Test
+		void positionOverridesDefault() {
+			register(ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7", "a");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("a");
+					assertThat(r.position()).isEqualTo(0);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				}
+			);
+		}
+
+		@Test
+		void noArgsGetsOnlyDefaults() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).isEmpty();
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("arg1default");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("arg2default");
+				}
+			);
+		}
+
+		@Test
+		void positionOverridesDefaultKeepsDefault() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7", "a");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("a");
+					assertThat(r.position()).isEqualTo(0);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("arg2default");
+				}
+			);
+		}
+
+		@Test
+		void positionOverridesDefaultsKeepsDefault() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7", "a", "b");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("a");
+					assertThat(r.position()).isEqualTo(0);
+				},
+				r -> {
+					assertThat(r.value()).isEqualTo("b");
+					assertThat(r.position()).isEqualTo(1);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("b");
+				}
+			);
+		}
+
+		@Test
+		void positionOverridesDefaultsKeepsDefaultWhenOption() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT);
+			ParseResult result = parse("root7", "--arg1", "a", "b");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("b");
+					assertThat(r.position()).isEqualTo(0);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("b");
+				}
+			);
+		}
+
+		@Test
+		void positionWithLastHavingNoDefault() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT);
+			ParseResult result;
+
+			result = parse("root7", "--arg1", "a", "--arg2", "b", "c");
+			assertThat(result.messageResults()).isEmpty();
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("c");
+					assertThat(r.position()).isEqualTo(0);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("b");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg3" });
+					assertThat(r.value()).isEqualTo("c");
+				}
+			);
+		}
+
+		@Test
+		void tooManyArgsInMiddleShouldCreateError() {
+			register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT);
+			ParseResult result;
+
+			result = parse("root7", "--arg1", "a", "b", "--arg2", "c", "d");
+			assertThat(result.messageResults()).satisfiesExactlyInAnyOrder(
+				message -> {
+					ParserAssertions.assertThat(message.parserMessage()).hasCode(2004).hasType(ParserMessage.Type.ERROR);
+				}
+			);
+			assertThat(result.argumentResults()).satisfiesExactly(
+				r -> {
+					assertThat(r.value()).isEqualTo("b");
+					assertThat(r.position()).isEqualTo(0);
+				},
+				r -> {
+					assertThat(r.value()).isEqualTo("d");
+					assertThat(r.position()).isEqualTo(0);
+				}
+			);
+			assertThat(result.optionResults()).isNotNull().satisfiesExactly(
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" });
+					assertThat(r.value()).isEqualTo("a");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" });
+					assertThat(r.value()).isEqualTo("c");
+				},
+				r -> {
+					assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg3" });
+					assertThat(r.value()).isEqualTo("b");
+				}
+			);
+		}
+	}
 }

spring-shell-samples/spring-shell-sample-e2e/src/main/java/org/springframework/shell/samples/e2e/PositionalArgumentsCommands.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright 2023 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.shell.samples.e2e;
+
+import org.springframework.shell.command.CommandRegistration.OptionArity;
+import org.springframework.shell.command.annotation.Command;
+import org.springframework.shell.command.annotation.Option;
+import org.springframework.shell.standard.ShellComponent;
+import org.springframework.shell.standard.ShellMethod;
+import org.springframework.shell.standard.ShellOption;
+
+public class PositionalArgumentsCommands {
+
+	@ShellComponent
+	public static class LegacyAnnotation extends BaseE2ECommands {
+
+		@ShellMethod(key = LEGACY_ANNO + "positional-args-1", group = GROUP)
+		public String testPositionalArgs1(
+				@ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg1,
+				@ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg2,
+				@ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg3
+		) {
+				return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3);
+		}
+
+		@ShellMethod(key = LEGACY_ANNO + "positional-args-2", group = GROUP)
+		public String testPositionalArgs2(
+				@ShellOption(arity = 1, defaultValue = "defaultArg1") String arg1,
+				@ShellOption(arity = 1, defaultValue = "defaultArg2") String arg2,
+				@ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg3
+		) {
+				return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3);
+		}
+	}
+
+	@Command(command = BaseE2ECommands.ANNO, group = BaseE2ECommands.GROUP)
+	public static class Annotation extends BaseE2ECommands {
+
+		@Command(command = "positional-args-1")
+		public String testPositionalArgs1(
+				@Option(arity = OptionArity.EXACTLY_ONE) String arg1,
+				@Option(arity = OptionArity.EXACTLY_ONE) String arg2,
+				@Option(arity = OptionArity.EXACTLY_ONE) String arg3
+		) {
+				return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3);
+		}
+
+		@Command(command = "positional-args-2")
+		public String testPositionalArgs2(
+				@Option(arity = OptionArity.EXACTLY_ONE, defaultValue = "defaultArg1") String arg1,
+				@Option(arity = OptionArity.EXACTLY_ONE, defaultValue = "defaultArg2") String arg2,
+				@Option(arity = OptionArity.EXACTLY_ONE) String arg3
+		) {
+				return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3);
+		}
+	}
+
+}