Reject less/greater than operators on booleans (#18206)

frontend/src/metabase/lib/expressions/typechecker.js

@@ -3,6 +3,8 @@ import { ngettext, msgid, t } from "ttag";
 import { ExpressionVisitor } from "./visitor";
 import { CLAUSE_TOKENS } from "./lexer";
 
+import { MBQL_CLAUSES, getMBQLName } from "./config";
+
 export function typeCheck(cst, rootType) {
   class TypeChecker extends ExpressionVisitor {
     constructor() {
@@ -30,7 +32,7 @@ export function typeCheck(cst, rootType) {
       return result;
     }
     relationalExpression(ctx) {
-      this.typeStack.unshift("expression");
+      this.typeStack.unshift("number");
       const result = super.relationalExpression(ctx);
       this.typeStack.shift();
 
@@ -79,6 +81,18 @@ export function typeCheck(cst, rootType) {
         );
         this.errors.push({ message });
       } else {
+        // check for return value sub-type mismatch
+        const type = this.typeStack[0];
+        if (type === "number") {
+          const op = getMBQLName(name);
+          const returnType = MBQL_CLAUSES[op].type;
+          if (returnType !== "number" && returnType !== "string") {
+            const message = t`Expecting ${type} but found function ${name} returning ${returnType}`;
+            this.errors.push({ message });
+          }
+        }
+
+        // check for argument type matching
         return args.map((arg, index) => {
           const argType = clause.args[index];
           const genericType =

frontend/test/metabase/lib/expressions/typechecker.unit.spec.js

@@ -176,6 +176,19 @@ describe("type-checker", () => {
       expect(() => validate("CONTAINS([Type],'X','Y')")).toThrow();
       expect(() => validate("CONTAINS([Type],'P','Q','R')")).toThrow();
     });
+
+    it("should allow a comparison (lexicographically) on strings", () => {
+      expect(() => validate("'A' <= 'B'")).not.toThrow();
+    });
+
+    it("should allow a comparison (lexicographically) on functions returning string", () => {
+      expect(() => validate("Lower([State]) > 'AB'")).not.toThrow();
+    });
+
+    it("should reject a less/greater comparison on functions returning boolean", () => {
+      expect(() => validate("IsEmpty([Tax]) < 5")).toThrow();
+      expect(() => validate("IsEmpty([Tax]) >= 0")).toThrow();
+    });
   });
 
   describe("for an aggregation", () => {