Fix check_unknown_options! when parsing gets stopped

If the parsing gets stopped by either a -- or by #stop_on_unknown_option!, then check_unknown_options! would still try to do validation on the unparsed parts.
We now track where the stop happened and only check the extras that were before that point.

Author: MaxLap

CHANGELOG.md

@@ -1,3 +1,5 @@
+* Fix `check_unknown_options!` to not check the content that was not parsed, i.e. after a `--` or after the first unknown with `stop_on_unknown_option!`
+
 ## 0.20.0
 * Add `check_default_type!` to check if the default value of an option matches the defined type.
   It removes the warning on usage and gives the command authors the possibility to check for programming errors.

lib/thor/parser/options.rb

@@ -44,6 +44,7 @@ def initialize(hash_options = {}, defaults = {}, stop_on_unknown = false, disabl
       @shorts = {}
       @switches = {}
       @extra = []
+      @stopped_parsing_after_extra_index = nil
 
       options.each do |option|
         @switches[option.switch_name] = option
@@ -66,6 +67,7 @@ def peek
       if result == OPTS_END
         shift
         @parsing_options = false
+        @stopped_parsing_after_extra_index ||= @extra.size
         super
       else
         result
@@ -99,6 +101,7 @@ def parse(args) # rubocop:disable MethodLength
           elsif @stop_on_unknown
             @parsing_options = false
             @extra << shifted
+            @stopped_parsing_after_extra_index ||= @extra.size
             @extra << shift while peek
             break
           elsif match
@@ -120,8 +123,10 @@ def parse(args) # rubocop:disable MethodLength
     end
 
     def check_unknown!
+      to_check = @stopped_parsing_after_extra_index ? @extra[0...@stopped_parsing_after_extra_index] : @extra
+
       # an unknown option starts with - or -- and has no more --'s afterward.
-      unknown = @extra.select { |str| str =~ /^--?(?:(?!--).)*$/ }
+      unknown = to_check.select { |str| str =~ /^--?(?:(?!--).)*$/ }
       raise UnknownArgumentError, "Unknown switches '#{unknown.join(', ')}'" unless unknown.empty?
     end
 

spec/thor_spec.rb

@@ -127,6 +127,10 @@ def boring(*args)
       expect(my_script.start(%w(exec -- --verbose))).to eq [{}, %w(--verbose)]
     end
 
+    it "still passes everything after -- to command, complex" do
+      expect(my_script.start(%w[exec command --mode z again -- --verbose more])).to eq [{}, %w[command --mode z again -- --verbose more]]
+    end
+
     it "does not affect ordinary commands" do
       expect(my_script.start(%w(boring command --verbose))).to eq [{"verbose" => true}, %w(command)]
     end
@@ -157,6 +161,113 @@ def boring(*args)
     it "doesn't break new" do
       expect(my_script.new).to be_a(Thor)
     end
+
+    context "along with check_unknown_options!" do
+      my_script2 = Class.new(Thor) do
+        class_option "verbose",   :type => :boolean
+        class_option "mode",      :type => :string
+        check_unknown_options!
+        stop_on_unknown_option! :exec
+
+        desc "exec", "Run a command"
+        def exec(*args)
+          [options, args]
+        end
+      end
+
+      it "passes remaining args to command when it encounters a non-option" do
+        expect(my_script2.start(%w[exec command --verbose])).to eq [{}, %w[command --verbose]]
+      end
+
+      it "does not accept if first non-option looks like an option, but only refuses that invalid option" do
+        expect(capture(:stderr) do
+          my_script2.start(%w[exec --foo command --bar])
+        end.strip).to eq("Unknown switches '--foo'")
+      end
+
+      it "still accepts options that are given before non-options" do
+        expect(my_script2.start(%w[exec --verbose command])).to eq [{"verbose" => true}, %w[command]]
+      end
+
+      it "still accepts when non-options are given after real options and argument" do
+        expect(my_script2.start(%w[exec --verbose command --foo])).to eq [{"verbose" => true}, %w[command --foo]]
+      end
+
+      it "does not accept when non-option looks like an option and is after real options" do
+        expect(capture(:stderr) do
+          my_script2.start(%w[exec --verbose --foo])
+        end.strip).to eq("Unknown switches '--foo'")
+      end
+
+      it "still accepts options that require a value" do
+        expect(my_script2.start(%w[exec --mode rashly command])).to eq [{"mode" => "rashly"}, %w[command]]
+      end
+
+      it "still passes everything after -- to command" do
+        expect(my_script2.start(%w[exec -- --verbose])).to eq [{}, %w[--verbose]]
+      end
+
+      it "still passes everything after -- to command, complex" do
+        expect(my_script2.start(%w[exec command --mode z again -- --verbose more])).to eq [{}, %w[command --mode z again -- --verbose more]]
+      end
+    end
+  end
+
+  describe "#check_unknown_options!" do
+    my_script = Class.new(Thor) do
+      class_option "verbose",   :type => :boolean
+      class_option "mode",      :type => :string
+      check_unknown_options!
+
+      desc "checked", "a command with checked"
+      def checked(*args)
+        [options, args]
+      end
+    end
+
+    it "still accept options and arguments" do
+      expect(my_script.start(%w[checked command --verbose])).to eq [{"verbose" => true}, %w[command]]
+    end
+
+    it "still accepts options that are given before arguments" do
+      expect(my_script.start(%w[checked --verbose command])).to eq [{"verbose" => true}, %w[command]]
+    end
+
+    it "does not accept if non-option that looks like an option is before the arguments" do
+      expect(capture(:stderr) do
+        my_script.start(%w[checked --foo command --bar])
+      end.strip).to eq("Unknown switches '--foo, --bar'")
+    end
+
+    it "does not accept if non-option that looks like an option is after an argument" do
+      expect(capture(:stderr) do
+        my_script.start(%w[checked command --foo --bar])
+      end.strip).to eq("Unknown switches '--foo, --bar'")
+    end
+
+    it "does not accept when non-option that looks like an option is after real options" do
+      expect(capture(:stderr) do
+        my_script.start(%w[checked --verbose --foo])
+      end.strip).to eq("Unknown switches '--foo'")
+    end
+
+    it "does not accept when non-option that looks like an option is before real options" do
+      expect(capture(:stderr) do
+        my_script.start(%w[checked --foo --verbose])
+      end.strip).to eq("Unknown switches '--foo'")
+    end
+
+    it "still accepts options that require a value" do
+      expect(my_script.start(%w[checked --mode rashly command])).to eq [{"mode" => "rashly"}, %w[command]]
+    end
+
+    it "still passes everything after -- to command" do
+      expect(my_script.start(%w[checked -- --verbose])).to eq [{}, %w[--verbose]]
+    end
+
+    it "still passes everything after -- to command, complex" do
+      expect(my_script.start(%w[checked command --mode z again -- --verbose more])).to eq [{"mode" => "z"}, %w[command again --verbose more]]
+    end
   end
 
   describe "#disable_required_check!" do