#147 implement use-simple-attribute (#628)

* implement use-simple-attribute

* fix typo in useSimpleAttributeRule
reorder use-simple-attribute in tslint.json

* add rationale to useSimpleAttributeRule

* Merge branch master; rename to plural rule

* Added description to README.md

README.md

@@ -934,6 +934,16 @@ We recommend you specify exact versions of lint libraries, including `tslint-mic
       </td>
       <td>6.0.0-beta</td>
     </tr>
+    <tr>
+      <td>
+        <code>use-simple-attributes</code>
+      </td>
+      <td>
+        Simpler attributes in JSX and TSX files helps keep code clean and readable.
+        Separate complex expressions into their own line and use clear variable names to make your code more understandable.
+      </td>
+      <td>6.0.0</td>
+    </tr>
     <tr>
       <td>
         <code>react-a11y-props</code>

recommended_ruleset.js

@@ -96,6 +96,7 @@ module.exports = {
         'triple-equals': [true, 'allow-null-check'],
         'use-isnan': true,
         'use-named-parameter': true,
+        'use-simple-attributes': true,
 
         /**
          * Code Clarity. The following rules should be turned on because they make the code

src/tests/useSimpleAttributeRuleTests.ts

@@ -0,0 +1,102 @@
+import { Utils } from '../utils/Utils';
+import { TestHelper } from './TestHelper';
+/**
+ * Unit tests.
+ */
+describe('useSimpleAttributesRule', (): void => {
+    const ruleName: string = 'use-simple-attributes';
+    const binaryExpressionErrorMessage: string = 'Attribute contains a complex binary expression';
+    const ternaryExpressionErrorMessage: string = 'Attribute contains a ternary expression';
+    it('should fail if only attribute initializer is a complex binary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const element = <foo bar={"value1" + "value2" + "value3"}/>\`;
+        `;
+
+        TestHelper.assertViolations(ruleName, script, [
+            {
+                failure: binaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 3 }
+            }
+        ]);
+    });
+    it('should fail if any attribute initializer is a complex binary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const element = <foo str="hello" bar={"value1" + "value2" + "value3"}/>\`;
+        `;
+
+        TestHelper.assertViolations(ruleName, script, [
+            {
+                failure: binaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 3 }
+            }
+        ]);
+    });
+    it('should pass if any attribute initializer is a simple binary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const element = <foo str="hello" bar={"value1" + "value2"}/>\`;
+        `;
+
+        TestHelper.assertNoViolation(ruleName, script);
+    });
+    it('should fail if only attribute initializer is a ternary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const someVar = 3;
+        const element = <foo bar={someVar == 3 ? true : false}/>\`;
+        `;
+
+        TestHelper.assertViolations(ruleName, script, [
+            {
+                failure: ternaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 4 }
+            }
+        ]);
+    });
+    it('should fail if any attribute initializer is a ternary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const someVar = 3;
+        const element = <foo str="123" bar={someVar == 3 ? true : false}/>\`;
+        `;
+
+        TestHelper.assertViolations(ruleName, script, [
+            {
+                failure: ternaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 4 }
+            }
+        ]);
+    });
+    it('should fail if any attribute initializer is a ternary expression or a complex binary expression', (): void => {
+        const script: string = `
+        import React = require('react');
+        const someVar = 3;
+        const element = <foo str="123" bar={someVar == 3 ? true : false} bin={"value1" + someVar + "value2"/>\`;
+        `;
+
+        TestHelper.assertViolations(ruleName, script, [
+            {
+                failure: ternaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 4 }
+            },
+            {
+                failure: binaryExpressionErrorMessage,
+                name: Utils.absolutePath('file.tsx'),
+                ruleName: ruleName,
+                startPosition: { character: 25, line: 4 }
+            }
+        ]);
+    });
+});

src/useSimpleAttributesRule.ts

@@ -0,0 +1,96 @@
+import * as ts from 'typescript';
+import * as Lint from 'tslint';
+
+import { ExtendedMetadata } from './utils/ExtendedMetadata';
+import { getJsxAttributesFromJsxElement } from './utils/JsxAttribute';
+
+export class Rule extends Lint.Rules.AbstractRule {
+    public static metadata: ExtendedMetadata = {
+        ruleName: 'use-simple-attributes',
+        type: 'functionality',
+        description: 'Enforce usage of only simple attribute types.',
+        rationale:
+            'Simpler attributes in JSX and TSX files helps keep code clean and readable.\
+            Separate complex expressions into their own line and use clear variable names to make your code more understandable.',
+        options: null, // tslint:disable-line:no-null-keyword
+        optionsDescription: '',
+        typescriptOnly: false,
+        issueClass: 'Non-SDL',
+        issueType: 'Error',
+        severity: 'Important',
+        level: 'Opportunity for Excellence',
+        group: 'Correctness'
+    };
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return sourceFile.languageVariant === ts.LanguageVariant.JSX
+            ? this.applyWithWalker(new UseSimpleAttributesRuleWalker(sourceFile, this.getOptions()))
+            : [];
+    }
+}
+
+class UseSimpleAttributesRuleWalker extends Lint.RuleWalker {
+    protected visitJsxSelfClosingElement(node: ts.JsxSelfClosingElement): void {
+        this.checkJsxOpeningElement(node);
+        super.visitJsxSelfClosingElement(node);
+    }
+
+    protected visitJsxElement(node: ts.JsxElement): void {
+        this.checkJsxOpeningElement(node.openingElement);
+        super.visitJsxElement(node);
+    }
+
+    private checkJsxOpeningElement(node: ts.JsxOpeningLikeElement) {
+        const attributes = getJsxAttributesFromJsxElement(node);
+        for (const key of Object.keys(attributes)) {
+            const attribute = attributes[key];
+
+            // Handle Binary Expressions
+            const binaryExpression = <ts.BinaryExpression>this.getNextNodeRecursive(attribute, ts.SyntaxKind.BinaryExpression);
+            if (binaryExpression && !this.isSimpleBinaryExpression(binaryExpression)) {
+                const binaryExpressionErrorMessage: string = 'Attribute contains a complex binary expression';
+                this.addFailureAt(node.getStart(), node.getWidth(), binaryExpressionErrorMessage);
+            }
+
+            // Handle Ternary Expression
+            const ternaryExpression = <ts.ConditionalExpression>this.getNextNodeRecursive(attribute, ts.SyntaxKind.ConditionalExpression);
+            if (ternaryExpression) {
+                const ternaryExpressionErrorMessage: string = 'Attribute contains a ternary expression';
+                this.addFailureAt(node.getStart(), node.getWidth(), ternaryExpressionErrorMessage);
+            }
+        }
+    }
+
+    private isSimpleBinaryExpression(binaryExpression: ts.BinaryExpression): boolean {
+        if (binaryExpression.kind !== ts.SyntaxKind.BinaryExpression) {
+            return false;
+        }
+
+        // Both children of a Binary Expression should be primitives, constants or identifiers
+        const allowedBinaryNodes: ts.SyntaxKind[] = [
+            ts.SyntaxKind.NumericLiteral,
+            ts.SyntaxKind.StringLiteral,
+            ts.SyntaxKind.TrueKeyword,
+            ts.SyntaxKind.FalseKeyword,
+            ts.SyntaxKind.Identifier
+        ];
+
+        const leftTerm = allowedBinaryNodes.find(kind => kind === binaryExpression.left.kind);
+        const rightTerm = allowedBinaryNodes.find(kind => kind === binaryExpression.right.kind);
+        return leftTerm ? (rightTerm ? true : false) : false;
+    }
+
+    private getNextNodeRecursive(node: ts.Node, kind: ts.SyntaxKind): ts.Node | undefined {
+        if (!node) {
+            return undefined;
+        }
+        const childNodes = node.getChildren();
+        let match = childNodes.find(cn => cn.kind === kind);
+        if (!match) {
+            for (const childNode of childNodes) {
+                match = this.getNextNodeRecursive(childNode, kind);
+            }
+        }
+        return match;
+    }
+}

tslint-warnings.csv

@@ -310,6 +310,7 @@ unified-signatures,Warns for any two overloads that could be unified into one by
 use-default-type-parameter,Warns if an explicitly specified type argument is the default for that type parameter.,TSLINTLMNGTP,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation"
 use-isnan,Enforces use of the `isNaN()` function to check for NaN references instead of a comparison to the `NaN` constant.,TSLINTPUV7LC,tslint,Non-SDL,Error,Critical,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,398,"CWE 398 - Indicator of Poor Code Quality"
 use-named-parameter,"Do not reference the arguments object by numerical index; instead, use a named parameter.",TSLINTKPEHQG,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation"
+use-simple-attributes,Enforce usage of only simple attribute types.,TSLINT1PG0L9J,tslint,Non-SDL,Error,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
 valid-typeof,Ensures that the results of typeof are compared against a valid string.,TSLINT1IB59P1,tslint,Non-SDL,Error,Critical,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
 variable-name,Checks variable names for various errors.,TSLINT1CIV7K3,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,"398, 710","CWE 398 - Indicator of Poor Code Quality
 CWE 710 - Coding Standards Violation"

tslint.json

@@ -155,6 +155,7 @@
         "underscore-consistent-invocation": true,
         "use-default-type-parameter": false,
         "use-named-parameter": true,
+        "use-simple-attributes": true,
 
         // tslint-microsoft-contrib rules disabled
         "missing-jsdoc": false,