Repository: rokucommunity/brighterscript
Pull request #896 (2023-09-19 03:02:42)

Adds validation for binary operators
Solves  #880

![image](https://github.com/rokucommunity/brighterscript/assets/810290/a6c8c704-2bfd-4739-90ed-cfb4d104f56d)



Patch:
diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts
index 0c8151c0..ccd2510d 100644
--- a/src/DiagnosticMessages.ts
+++ b/src/DiagnosticMessages.ts
@@ -744,6 +744,11 @@ export let DiagnosticMessages = {
         message: `Type '${actualTypeString}' is not compatible with type '${expectedTypeString}'`,
         code: 1143,
         severity: DiagnosticSeverity.Error
+    }),
+    operatorTypeMismatch: (operatorString: string, firstType: string, secondType = '') => ({
+        message: `Operator '${operatorString}' cannot be applied to type${secondType ? 's' : ''} '${firstType}'${secondType ? ` and '${secondType}'` : ''}`,
+        code: 1144,
+        severity: DiagnosticSeverity.Error
     })
 };
 
diff --git a/src/astUtils/reflection.ts b/src/astUtils/reflection.ts
index 705166c7..a60ae298 100644
--- a/src/astUtils/reflection.ts
+++ b/src/astUtils/reflection.ts
@@ -10,10 +10,10 @@ import type { TypedFunctionType } from '../types/TypedFunctionType';
 import type { FunctionType } from '../types/FunctionType';
 import type { StringType } from '../types/StringType';
 import type { BooleanType } from '../types/BooleanType';
-import { IntegerType } from '../types/IntegerType';
-import { LongIntegerType } from '../types/LongIntegerType';
-import { FloatType } from '../types/FloatType';
-import { DoubleType } from '../types/DoubleType';
+import type { IntegerType } from '../types/IntegerType';
+import type { LongIntegerType } from '../types/LongIntegerType';
+import type { FloatType } from '../types/FloatType';
+import type { DoubleType } from '../types/DoubleType';
 import type { ClassType } from '../types/ClassType';
 import type { Scope } from '../Scope';
 import type { XmlScope } from '../XmlScope';
@@ -351,16 +351,29 @@ export function isAnyReferenceType(target): target is ReferenceType | TypeProper
     return name === 'ReferenceType' || name === 'TypePropertyReferenceType' || name === 'BinaryOperatorReferenceType';
 }
 
-const numberConstructorNames = [
-    IntegerType.name,
-    LongIntegerType.name,
-    FloatType.name,
-    DoubleType.name
+const numberTypeKinds = [
+    BscTypeKind.IntegerType,
+    BscTypeKind.LongIntegerType,
+    BscTypeKind.FloatType,
+    BscTypeKind.DoubleType
 ];
 export function isNumberType(value: any): value is IntegerType | LongIntegerType | FloatType | DoubleType {
-    return numberConstructorNames.includes(value?.constructor.name);
+    return numberTypeKinds.includes(value?.kind);
 }
 
+const primitiveTypeKinds = [
+    BscTypeKind.IntegerType,
+    BscTypeKind.LongIntegerType,
+    BscTypeKind.FloatType,
+    BscTypeKind.DoubleType,
+    BscTypeKind.BooleanType,
+    BscTypeKind.StringType
+];
+export function isPrimitiveType(value: any): value is IntegerType | LongIntegerType | FloatType | DoubleType | StringType | BooleanType {
+    return primitiveTypeKinds.includes(value?.kind);
+}
+
+
 // Literal reflection
 
 export function isLiteralInvalid(value: any): value is LiteralExpression & { type: InvalidType } {
diff --git a/src/bscPlugin/validation/ScopeValidator.spec.ts b/src/bscPlugin/validation/ScopeValidator.spec.ts
index ce0cd724..d927fadf 100644
--- a/src/bscPlugin/validation/ScopeValidator.spec.ts
+++ b/src/bscPlugin/validation/ScopeValidator.spec.ts
@@ -1059,7 +1059,7 @@ describe('ScopeValidator', () => {
     });
 
     describe('assignmentTypeMismatch', () => {
-        it('finds when the type of the lhs is not compatible with teh expected type', () => {
+        it('finds when the type of the lhs is not compatible with the expected type', () => {
             program.setFile('source/util.bs', `
                 sub doStuff(thing as iThing)
                     thing.name = 123
@@ -1158,5 +1158,167 @@ describe('ScopeValidator', () => {
             ]);
         });
 
+        it('validates when assigning to a sgNode', () => {
+            program.setFile('source/util.bs', `
+                sub setLabelText(label as roSGNodeLabel)
+                    label.text = 1234
+                end sub
+
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.assignmentTypeMismatch('integer', 'string').message
+            ]);
+        });
+
+    });
+
+    describe('operatorTypeMismatch', () => {
+        it('finds when the type of the lhs is not compatible with the rhs type', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff()
+                    a = 1 + true
+                    b = "hello" * 2
+                end sub
+
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.operatorTypeMismatch('+', 'integer', 'boolean').message,
+                DiagnosticMessages.operatorTypeMismatch('*', 'string', 'integer').message
+            ]);
+        });
+
+        it('allows when the type of the lhs is compatible with the rhs type', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff()
+                    a = 10 << 1
+                    b = "hello" + "world"
+                    c = 78 / 34
+                    d = 100 \\ 5
+                    thing = new Klass()
+                    e = thing <> invalid
+                end sub
+
+                class Klass
+                end class
+            `);
+            program.validate();
+            //should have no errors
+            expectZeroDiagnostics(program);
+        });
+
+        it('allows tests against invalid', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff()
+                    thing = new Klass()
+                    x = thing <> invalid
+                end sub
+
+                class Klass
+                end class
+            `);
+            program.validate();
+            //should have no errors
+            expectZeroDiagnostics(program);
+        });
+
+        it('disallows equality tests of classes', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff()
+                    thing = new Klass()
+                    thing2 = new Klass()
+                    x = thing = thing2
+                end sub
+
+                class Klass
+                end class
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.operatorTypeMismatch('=', 'Klass', 'Klass').message
+            ]);
+        });
+
+        it('disallows operations between dynamic and custom types', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff(input)
+                    thing = new Klass()
+                    x = thing + input
+                end sub
+
+                class Klass
+                end class
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.operatorTypeMismatch('+', 'Klass', 'dynamic').message
+            ]);
+        });
+
+        it('allows valid operations on enum members', () => {
+            program.setFile('source/util.bs', `
+                sub makeEasterly(d as Direction)
+                    print d + "e"
+                    print Direction.north + "east"
+                end sub
+
+                function getTax(itemAmt as ItemCost) as Float
+                    return itemAmt * 1.15
+                end function
+
+                enum Direction
+                    north = "n"
+                    south = "s"
+                end enum
+
+                enum ItemCost
+                    x = 99.99
+                    y = 29.99
+                end enum
+
+            `);
+            program.validate();
+            //should have no errors
+            expectZeroDiagnostics(program);
+        });
+
+        it('finds invalid operations on enum members', () => {
+            program.setFile('source/util.bs', `
+                enum Direction
+                    north = "n"
+                    south = "s"
+                end enum
+
+                sub makeEasterly(d as Direction)
+                    print d + 2
+                    print 3.14 * Direction.north
+                end sub
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.operatorTypeMismatch('+', 'Direction', 'integer').message,
+                DiagnosticMessages.operatorTypeMismatch('*', 'float', 'Direction').message
+            ]);
+        });
+
+        it('validates unary operators', () => {
+            program.setFile('source/util.bs', `
+                sub doStuff()
+                    x = - "hello world"
+                end sub
+            `);
+            program.validate();
+            //should have errors
+            expectDiagnostics(program, [
+                DiagnosticMessages.operatorTypeMismatch('-', 'string').message
+            ]);
+        });
+
     });
 });
diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts
index 59e5f20b..1aa32d98 100644
--- a/src/bscPlugin/validation/ScopeValidator.ts
+++ b/src/bscPlugin/validation/ScopeValidator.ts
@@ -1,5 +1,5 @@
 import { URI } from 'vscode-uri';
-import { isAssignmentStatement, isBinaryExpression, isBrsFile, isClassType, isFunctionExpression, isLiteralExpression, isNamespaceStatement, isTypeExpression, isTypedFunctionType, isUnionType, isVariableExpression, isXmlScope } from '../../astUtils/reflection';
+import { isAssignmentStatement, isBinaryExpression, isBrsFile, isClassType, isDynamicType, isEnumMemberType, isEnumType, isFunctionExpression, isLiteralExpression, isNamespaceStatement, isObjectType, isPrimitiveType, isTypeExpression, isTypedFunctionType, isUnionType, isVariableExpression, isXmlScope } from '../../astUtils/reflection';
 import { Cache } from '../../Cache';
 import { DiagnosticMessages } from '../../DiagnosticMessages';
 import type { BrsFile } from '../../files/BrsFile';
@@ -13,7 +13,7 @@ import type { Token } from '../../lexer/Token';
 import type { Scope } from '../../Scope';
 import type { DiagnosticRelatedInformation } from 'vscode-languageserver';
 import { type AstNode, type Expression } from '../../parser/AstNode';
-import type { VariableExpression, DottedGetExpression, CallExpression } from '../../parser/Expression';
+import type { VariableExpression, DottedGetExpression, CallExpression, BinaryExpression, UnaryExpression } from '../../parser/Expression';
 import { ParseMode } from '../../parser/Parser';
 import { TokenKind } from '../../lexer/TokenKind';
 import { WalkMode, createVisitor } from '../../astUtils/visitors';
@@ -64,6 +64,12 @@ export class ScopeValidator {
                     },
                     DottedSetStatement: (dottedSetStmt) => {
                         this.validateDottedSetStatement(file, dottedSetStmt);
+                    },
+                    BinaryExpression: (binaryExpr) => {
+                        this.validateBinaryExpression(file, binaryExpr);
+                    },
+                    UnaryExpression: (unaryExpr) => {
+                        this.validateUnaryExpression(file, unaryExpr);
                     }
 
                 }), {
@@ -508,6 +514,107 @@ export class ScopeValidator {
         this.event.scope.addDiagnostics(diagnostics);
     }
 
+    /**
+     * Detect invalid use of a binary operator
+     */
+    private validateBinaryExpression(file: BrsFile, binaryExpr: BinaryExpression) {
+        const diagnostics: BsDiagnostic[] = [];
+        const getTypeOpts = { flags: SymbolTypeFlag.runtime };
+
+        if (this.checkIfUsedAsTypeExpression(binaryExpr)) {
+            return;
+        }
+
+        let leftType = binaryExpr.left.getType(getTypeOpts);
+        let rightType = binaryExpr.right.getType(getTypeOpts);
+
+        if (!leftType.isResolvable() || !rightType.isResolvable()) {
+            // Can not find the type. error handled elsewhere
+            return;
+        }
+        let leftTypeToTest = leftType;
+        let rightTypeToTest = rightType;
+
+        if (isEnumMemberType(leftType) || isEnumType(leftType)) {
+            leftTypeToTest = leftType.underlyingType;
+        }
+        if (isEnumMemberType(rightType) || isEnumType(rightType)) {
+            rightTypeToTest = rightType.underlyingType;
+        }
+
+        if (isUnionType(leftType) || isUnionType(rightType)) {
+            // TODO: it is possible to validate based on innerTypes, but more complicated
+            // Because you need to verify each combination of types
+            return;
+        }
+        const leftIsPrimitive = isPrimitiveType(leftTypeToTest);
+        const rightIsPrimitive = isPrimitiveType(rightTypeToTest);
+        const leftIsAny = isDynamicType(leftTypeToTest) || isObjectType(leftTypeToTest);
+        const rightIsAny = isDynamicType(rightTypeToTest) || isObjectType(rightTypeToTest);
+
+
+        if (leftIsAny && rightIsAny) {
+            // both operands are basically "any" type... ignore;
+            return;
+        } else if ((leftIsAny && rightIsPrimitive) || (leftIsPrimitive && rightIsAny)) {
+            // one operand is basically "any" type... ignore;
+            return;
+        }
+        const opResult = util.binaryOperatorResultType(leftTypeToTest, binaryExpr.operator, rightTypeToTest);
+
+        if (isDynamicType(opResult)) {
+            // if the result was dynamic, that means there wasn't a valid operation
+            this.addMultiScopeDiagnostic({
+                ...DiagnosticMessages.operatorTypeMismatch(binaryExpr.operator.text, leftType.toString(), rightType.toString()),
+                range: binaryExpr.range,
+                file: file
+            });
+        }
+
+        this.event.scope.addDiagnostics(diagnostics);
+    }
+
+    /**
+     * Detect invalid use of a Unary operator
+     */
+    private validateUnaryExpression(file: BrsFile, unaryExpr: UnaryExpression) {
+        const diagnostics: BsDiagnostic[] = [];
+        const getTypeOpts = { flags: SymbolTypeFlag.runtime };
+
+        let rightType = unaryExpr.right.getType(getTypeOpts);
+
+        if (!rightType.isResolvable()) {
+            // Can not find the type. error handled elsewhere
+            return;
+        }
+        let rightTypeToTest = rightType;
+        if (isEnumMemberType(rightType)) {
+            rightTypeToTest = rightType.underlyingType;
+        }
+        if (isDynamicType(rightTypeToTest) || isObjectType(rightTypeToTest)) {
+            // operand is basically "any" type... ignore;
+            return;
+        }
+        if (isPrimitiveType(rightType)) {
+            const opResult = util.unaryOperatorResultType(unaryExpr.operator, rightTypeToTest);
+            if (isDynamicType(opResult)) {
+                this.addMultiScopeDiagnostic({
+                    ...DiagnosticMessages.operatorTypeMismatch(unaryExpr.operator.text, rightType.toString()),
+                    range: unaryExpr.range,
+                    file: file
+                });
+            }
+        } else {
+            // rhs is not a primitive, so no binary operator is allowed
+            this.addMultiScopeDiagnostic({
+                ...DiagnosticMessages.operatorTypeMismatch(unaryExpr.operator.text, rightType.toString()),
+                range: unaryExpr.range,
+                file: file
+            });
+        }
+        this.event.scope.addDiagnostics(diagnostics);
+    }
+
     /**
      * Adds a diagnostic to the first scope for this key. Prevents duplicate diagnostics
      * for diagnostics where scope isn't important. (i.e. CreateObject validations)
@@ -549,6 +656,7 @@ export class ScopeValidator {
                 diagnostic.range
             );
         }
+
         diagnostic.relatedInformation.push(info);
     }
 
diff --git a/src/globalCallables.ts b/src/globalCallables.ts
index 65b29e16..3910edb4 100644
--- a/src/globalCallables.ts
+++ b/src/globalCallables.ts
@@ -814,7 +814,7 @@ let programStatementFunctions = [
     }, {
         name: 'Pos',
         shortDescription: 'Returns a number from 0 to window width, indicating the current cursor position on the cursor. Requires a "dummy argument" (any numeric expression).',
-        type: new TypedFunctionType(new VoidType()),
+        type: new TypedFunctionType(new IntegerType()),
         file: globalFile,
         params: [{
             name: 'x',
diff --git a/src/parser/Statement.ts b/src/parser/Statement.ts
index 4fb129c9..55c9c224 100644
--- a/src/parser/Statement.ts
+++ b/src/parser/Statement.ts
@@ -2687,7 +2687,7 @@ export class EnumStatement extends Statement implements TypedefProvider {
 
         const resultType = new EnumType(
             this.fullName,
-            members[0]?.getType(options)
+            members[0]?.getType(options).underlyingType
         );
         resultType.pushMemberProvider(() => this.getSymbolTable());
         for (const statement of members) {
diff --git a/src/util.ts b/src/util.ts
index 1dc14ecb..43995688 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -24,7 +24,7 @@ import type { CallExpression, CallfuncExpression, DottedGetExpression, FunctionP
 import { Logger, LogLevel } from './Logger';
 import type { Identifier, Locatable, Token } from './lexer/Token';
 import { TokenKind } from './lexer/TokenKind';
-import { isAnyReferenceType, isBooleanType, isBrsFile, isCallExpression, isCallfuncExpression, isDottedGetExpression, isDoubleType, isDynamicType, isExpression, isFloatType, isIndexedGetExpression, isIntegerType, isInvalidType, isLongIntegerType, isStringType, isTypeExpression, isVariableExpression, isXmlAttributeGetExpression, isXmlFile } from './astUtils/reflection';
+import { isAnyReferenceType, isBooleanType, isBrsFile, isCallExpression, isCallfuncExpression, isDottedGetExpression, isDoubleType, isDynamicType, isEnumMemberType, isExpression, isFloatType, isIndexedGetExpression, isInvalidType, isLongIntegerType, isNumberType, isStringType, isTypeExpression, isVariableExpression, isXmlAttributeGetExpression, isXmlFile } from './astUtils/reflection';
 import { WalkMode } from './astUtils/visitors';
 import { SourceNode } from 'source-map';
 import * as requireRelative from 'require-relative';
@@ -1217,15 +1217,6 @@ export class Util {
         return DynamicType.instance;
     }
 
-
-    public isNumberType(targetType: BscType): boolean {
-        return isIntegerType(targetType) ||
-            isFloatType(targetType) ||
-            isDoubleType(targetType) ||
-            isLongIntegerType(targetType);
-    }
-
-
     /**
      * Return the type of the result of a binary operator
      * Note: compound assignments (eg. +=) internally use a binary expression, so that's why TokenKind.PlusEqual, etc. are here too
@@ -1237,14 +1228,20 @@ export class Util {
                 return this.binaryOperatorResultType(lhs, op, rhs);
             });
         }
+        if (isEnumMemberType(leftType)) {
+            leftType = leftType.underlyingType;
+        }
+        if (isEnumMemberType(rightType)) {
+            rightType = rightType.underlyingType;
+        }
         let hasDouble = isDoubleType(leftType) || isDoubleType(rightType);
         let hasFloat = isFloatType(leftType) || isFloatType(rightType);
         let hasLongInteger = isLongIntegerType(leftType) || isLongIntegerType(rightType);
         let hasInvalid = isInvalidType(leftType) || isInvalidType(rightType);
         let hasDynamic = isDynamicType(leftType) || isDynamicType(rightType);
-        let bothNumbers = this.isNumberType(leftType) && this.isNumberType(rightType);
+        let bothNumbers = isNumberType(leftType) && isNumberType(rightType);
         let bothStrings = isStringType(leftType) && isStringType(rightType);
-        let eitherBooleanOrNum = (this.isNumberType(leftType) || isBooleanType(leftType)) && (this.isNumberType(rightType) || isBooleanType(rightType));
+        let eitherBooleanOrNum = (isNumberType(leftType) || isBooleanType(leftType)) && (isNumberType(rightType) || isBooleanType(rightType));
 
         // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check
         switch (operator.kind) {
@@ -1356,7 +1353,7 @@ export class Util {
         switch (operator.kind) {
             // Math operators
             case TokenKind.Minus:
-                if (this.isNumberType(exprType)) {
+                if (isNumberType(exprType)) {
                     // a negative number will be the same type, eg, double->double, int->int, etc.
                     return exprType;
                 }
@@ -1364,7 +1361,7 @@ export class Util {
             case TokenKind.Not:
                 if (isBooleanType(exprType)) {
                     return BooleanType.instance;
-                } else if (this.isNumberType(exprType)) {
+                } else if (isNumberType(exprType)) {
                     //numbers can be "notted"
                     // by default they go to ints, except longints, which stay that way
                     if (isLongIntegerType(exprType)) {


Test patch:
diff --git a/src/parser/tests/statement/ConstStatement.spec.ts b/src/parser/tests/statement/ConstStatement.spec.ts
index 65c67403..f8dc3cf7 100644
--- a/src/parser/tests/statement/ConstStatement.spec.ts
+++ b/src/parser/tests/statement/ConstStatement.spec.ts
@@ -32,7 +32,7 @@ describe('ConstStatement', () => {
                 const = {
                     name: "Bob"
                 }
-                print const.name = {}
+                print const.name = "John"
             end sub
         `);
         program.validate();