From 8e37808d5f4891da3c815c5d8e059f6aa4b9d00c Mon Sep 17 00:00:00 2001
From: TheDiveO <6920158+thediveo@users.noreply.github.com>
Date: Tue, 26 Jul 2022 01:51:20 +0200
Subject: [PATCH] vet optional description args in assertions, fixing #560
 (#566)

---
 internal/assertion.go            |  5 +++++
 internal/assertion_test.go       | 30 ++++++++++++++++++++++++++++++
 internal/async_assertion.go      |  2 ++
 internal/async_assertion_test.go | 30 ++++++++++++++++++++++++++++++
 internal/vetoptdesc.go           | 22 ++++++++++++++++++++++
 5 files changed, 89 insertions(+)
 create mode 100644 internal/vetoptdesc.go

diff --git a/internal/assertion.go b/internal/assertion.go
index b3c26889a..7b7bdd149 100644
--- a/internal/assertion.go
+++ b/internal/assertion.go
@@ -45,26 +45,31 @@ func (assertion *Assertion) Error() types.Assertion {
 
 func (assertion *Assertion) Should(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *Assertion) ShouldNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 
 func (assertion *Assertion) To(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *Assertion) ToNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 
 func (assertion *Assertion) NotTo(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 
diff --git a/internal/assertion_test.go b/internal/assertion_test.go
index 83552f262..d008ea1d0 100644
--- a/internal/assertion_test.go
+++ b/internal/assertion_test.go
@@ -2,6 +2,7 @@ package internal_test
 
 import (
 	"errors"
+	"reflect"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -189,4 +190,33 @@ var _ = Describe("Making Synchronous Assertions", func() {
 		),
 	)
 
+	When("vetting optional description parameters", func() {
+
+		It("panics when Gomega matcher is at the beginning of optional description parameters", func() {
+			ig := NewInstrumentedGomega()
+			for _, expectator := range []string{
+				"To", "NotTo", "ToNot",
+				"Should", "ShouldNot",
+			} {
+				Expect(func() {
+					expect := ig.G.Expect(42) // sic!
+					meth := reflect.ValueOf(expect).MethodByName(expectator)
+					Expect(meth.IsValid()).To(BeTrue())
+					meth.Call([]reflect.Value{
+						reflect.ValueOf(HaveLen(1)),
+						reflect.ValueOf(ContainElement(42)),
+					})
+				}).To(PanicWith(MatchRegexp("Assertion has a GomegaMatcher as the first element of optionalDescription")))
+			}
+		})
+
+		It("accepts Gomega matchers in optional description parameters after the first", func() {
+			Expect(func() {
+				ig := NewInstrumentedGomega()
+				ig.G.Expect(42).To(HaveLen(1), "foo", ContainElement(42))
+			}).NotTo(Panic())
+		})
+
+	})
+
 })
diff --git a/internal/async_assertion.go b/internal/async_assertion.go
index 99f4ebcfe..6640ff15f 100644
--- a/internal/async_assertion.go
+++ b/internal/async_assertion.go
@@ -104,11 +104,13 @@ func (assertion *AsyncAssertion) WithPolling(interval time.Duration) types.Async
 
 func (assertion *AsyncAssertion) Should(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Asynchronous assertion", optionalDescription...)
 	return assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *AsyncAssertion) ShouldNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Asynchronous assertion", optionalDescription...)
 	return assertion.match(matcher, false, optionalDescription...)
 }
 
diff --git a/internal/async_assertion_test.go b/internal/async_assertion_test.go
index 970fa6662..3c67520b0 100644
--- a/internal/async_assertion_test.go
+++ b/internal/async_assertion_test.go
@@ -2,6 +2,7 @@ package internal_test
 
 import (
 	"errors"
+	"reflect"
 	"runtime"
 	"time"
 
@@ -712,4 +713,33 @@ var _ = Describe("Asynchronous Assertions", func() {
 			Ω(ig.FailureMessage).Should(ContainSubstring("Timed out after"))
 		})
 	})
+
+	When("vetting optional description parameters", func() {
+
+		It("panics when Gomega matcher is at the beginning of optional description parameters", func() {
+			ig := NewInstrumentedGomega()
+			for _, expectator := range []string{
+				"Should", "ShouldNot",
+			} {
+				Expect(func() {
+					eventually := ig.G.Eventually(42) // sic!
+					meth := reflect.ValueOf(eventually).MethodByName(expectator)
+					Expect(meth.IsValid()).To(BeTrue())
+					meth.Call([]reflect.Value{
+						reflect.ValueOf(HaveLen(1)),
+						reflect.ValueOf(ContainElement(42)),
+					})
+				}).To(PanicWith(MatchRegexp("Asynchronous assertion has a GomegaMatcher as the first element of optionalDescription")))
+			}
+		})
+
+		It("accepts Gomega matchers in optional description parameters after the first", func() {
+			Expect(func() {
+				ig := NewInstrumentedGomega()
+				ig.G.Eventually(42).Should(HaveLen(1), "foo", ContainElement(42))
+			}).NotTo(Panic())
+		})
+
+	})
+
 })
diff --git a/internal/vetoptdesc.go b/internal/vetoptdesc.go
new file mode 100644
index 000000000..f29587641
--- /dev/null
+++ b/internal/vetoptdesc.go
@@ -0,0 +1,22 @@
+package internal
+
+import (
+	"fmt"
+
+	"github.com/onsi/gomega/types"
+)
+
+// vetOptionalDescription vets the optional description args: if it finds any
+// Gomega matcher at the beginning it panics. This allows for rendering Gomega
+// matchers as part of an optional Description, as long as they're not in the
+// first slot.
+func vetOptionalDescription(assertion string, optionalDescription ...interface{}) {
+	if len(optionalDescription) == 0 {
+		return
+	}
+	if _, isGomegaMatcher := optionalDescription[0].(types.GomegaMatcher); isGomegaMatcher {
+		panic(fmt.Sprintf("%s has a GomegaMatcher as the first element of optionalDescription.\n\t"+
+			"Do you mean to use And/Or/SatisfyAll/SatisfyAny to combine multiple matchers?",
+			assertion))
+	}
+}