SONARJS-67 Rule: Variables should not be declared and then immediatel…

…y returned or thrown (#429)

javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java

@@ -115,6 +115,7 @@ public static List<Class> getChecks() {
       IdChildrenSelectorCheck.class,
       IdenticalExpressionOnBinaryOperatorCheck.class,
       IfConditionalAlwaysTrueOrFalseCheck.class,
+      ImmediatelyReturnedVariableCheck.class,
       IncrementDecrementInSubExpressionCheck.class,
       IndexOfCompareToPositiveNumberCheck.class,
       InOperatorTypeErrorCheck.class,

javascript-checks/src/main/java/org/sonar/javascript/checks/ImmediatelyReturnedVariableCheck.java

@@ -0,0 +1,110 @@
+/*
+ * SonarQube JavaScript Plugin
+ * Copyright (C) 2011-2016 SonarSource SA
+ * mailto:contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.javascript.checks;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.sonar.check.Rule;
+import org.sonar.javascript.tree.impl.SeparatedList;
+import org.sonar.plugins.javascript.api.tree.Tree.Kind;
+import org.sonar.plugins.javascript.api.tree.declaration.BindingElementTree;
+import org.sonar.plugins.javascript.api.tree.declaration.InitializedBindingElementTree;
+import org.sonar.plugins.javascript.api.tree.expression.ExpressionTree;
+import org.sonar.plugins.javascript.api.tree.expression.IdentifierTree;
+import org.sonar.plugins.javascript.api.tree.statement.BlockTree;
+import org.sonar.plugins.javascript.api.tree.statement.ReturnStatementTree;
+import org.sonar.plugins.javascript.api.tree.statement.StatementTree;
+import org.sonar.plugins.javascript.api.tree.statement.ThrowStatementTree;
+import org.sonar.plugins.javascript.api.tree.statement.VariableDeclarationTree;
+import org.sonar.plugins.javascript.api.tree.statement.VariableStatementTree;
+import org.sonar.plugins.javascript.api.visitors.DoubleDispatchVisitorCheck;
+
+@Rule(key = "S1488")
+public class ImmediatelyReturnedVariableCheck extends DoubleDispatchVisitorCheck {
+
+  private static final String MESSAGE = "Immediately %s this expression instead of assigning it to the temporary variable \"%s\".";
+
+  @Override
+  public void visitBlock(BlockTree tree) {
+    List<StatementTree> statements = tree.statements();
+
+    if (statements.size() > 1) {
+
+      StatementTree lastButOneStatement = statements.get(statements.size() - 2);
+      StatementTree lastStatement = statements.get(statements.size() - 1);
+
+      if (lastButOneStatement.is(Kind.VARIABLE_STATEMENT)) {
+        checkStatements(((VariableStatementTree) lastButOneStatement).declaration(), lastStatement);
+      }
+    }
+
+    super.visitBlock(tree);
+  }
+
+  private void checkStatements(VariableDeclarationTree variableDeclaration, StatementTree lastStatement) {
+    SeparatedList<BindingElementTree> variables = variableDeclaration.variables();
+
+    if (variables.size() == 1 && variables.get(0).is(Kind.INITIALIZED_BINDING_ELEMENT)) {
+      InitializedBindingElementTree initializedBindingElementTree = (InitializedBindingElementTree) variables.get(0);
+
+      if (initializedBindingElementTree.left().is(Kind.BINDING_IDENTIFIER)) {
+        String name = ((IdentifierTree) initializedBindingElementTree.left()).name();
+
+        if (returnsVariableInLastStatement(lastStatement, name)) {
+          addIssue(initializedBindingElementTree.right(), String.format(MESSAGE, "return", name));
+
+        } else if (throwsVariableInLastStatement(lastStatement, name)) {
+          addIssue(initializedBindingElementTree.right(), String.format(MESSAGE, "throw", name));
+
+        }
+      }
+    }
+  }
+
+  private static boolean returnsVariableInLastStatement(StatementTree lastStatement, String variableName) {
+    if (lastStatement.is(Kind.RETURN_STATEMENT)) {
+      ReturnStatementTree returnStatement = (ReturnStatementTree) lastStatement;
+
+      return isVariable(returnStatement.expression(), variableName);
+    }
+
+    return false;
+  }
+
+  private static boolean throwsVariableInLastStatement(StatementTree lastStatement, String variableName) {
+    if (lastStatement.is(Kind.THROW_STATEMENT)) {
+      ThrowStatementTree throwStatement = (ThrowStatementTree) lastStatement;
+      return isVariable(throwStatement.expression(), variableName);
+    }
+
+    return false;
+  }
+
+  private static boolean isVariable(@Nullable ExpressionTree expressionTree, String variableName) {
+    if (expressionTree != null && expressionTree.is(Kind.IDENTIFIER_REFERENCE)) {
+      String thrownName = ((IdentifierTree) expressionTree).name();
+      return thrownName.equals(variableName);
+    }
+
+    return false;
+  }
+
+
+}

javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1488.html

@@ -0,0 +1,18 @@
+<p>Declaring a variable only to immediately return or throw it is a bad practice.</p>
+<p>Some developers argue that the practice improves code readability, because it enables them to explicitly name what is being returned. However, this
+variable is an internal implementation detail that is not exposed to the callers of the method. The method name should be sufficient for callers to
+know exactly what will be returned.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+function computeDurationInMilliseconds() {
+  var duration = (((hours * 60) + minutes) * 60 + seconds ) * 1000 ;
+  return duration;
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+function computeDurationInMilliseconds() {
+  return (((hours * 60) + minutes) * 60 + seconds ) * 1000 ;
+}
+</pre>
+

javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1488.json

@@ -0,0 +1,13 @@
+{
+  "title": "Local Variables should not be declared and then immediately returned or thrown",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "2min"
+  },
+  "tags": [
+    "clumsy"
+  ],
+  "defaultSeverity": "Minor"
+}

javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json

@@ -41,6 +41,7 @@
     "S1301",
     "S1442",
     "S1472",
+    "S1488",
     "S1656",
     "S1751",
     "S1764",

javascript-checks/src/test/java/org/sonar/javascript/checks/ImmediatelyReturnedVariableCheckTest.java

@@ -0,0 +1,32 @@
+/*
+ * SonarQube JavaScript Plugin
+ * Copyright (C) 2011-2016 SonarSource SA
+ * mailto:contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.javascript.checks;
+
+import java.io.File;
+import org.junit.Test;
+import org.sonar.javascript.checks.verifier.JavaScriptCheckVerifier;
+
+public class ImmediatelyReturnedVariableCheckTest {
+
+  @Test
+  public void test() {
+    JavaScriptCheckVerifier.verify(new ImmediatelyReturnedVariableCheck(), new File("src/test/resources/checks/ImmediatelyReturnedVariable.js"));
+  }
+}

javascript-checks/src/test/resources/checks/ImmediatelyReturnedVariable.js

@@ -0,0 +1,120 @@
+function var_returned() {
+  var x = 42; // Noncompliant {{Immediately return this expression instead of assigning it to the temporary variable "x".}}
+//        ^^
+  return x;
+}
+
+function let_returned() {
+  let x = 42; // Noncompliant
+  return x;
+}
+
+function const_returned() {
+  const x = 42; // Noncompliant
+  return x;
+}
+
+function code_before_declaration() {
+  foo();
+  var x = 42; // Noncompliant
+  return x;
+}
+
+function thrown_nok() {
+  const x = new Exception(); // Noncompliant {{Immediately throw this expression instead of assigning it to the temporary variable "x".}}
+  throw x;
+}
+
+function thrown_ok() {
+  throw new Exception();
+}
+
+function thrown_expression() {
+  const x = new Exception();
+  throw foo(x);
+}
+
+function thrown_different_variable() {
+  const x = new Exception();
+  throw y;
+}
+
+function code_between_declaration_and_return() {
+  let x = 42;
+  foo();
+  return x;
+}
+
+function return_expression() {
+  let x = 42;
+  return x + 5;
+}
+
+function return_without_value() {
+  let x = 42;
+  return;
+}
+
+function not_return_statement() {
+  let x = 42;
+  foo(x);
+}
+
+function no_init_value() {
+  let x;
+  return x;
+}
+
+function pattern_declared() {
+  let {x} = foo();
+  return x;
+}
+
+function two_variables_declared() {
+  let x = 42, y;
+  return x;
+}
+
+function different_variable_returned() {
+  let x = 42;
+  return y;
+}
+
+function only_return() {
+  return 42;
+}
+
+function one_statement() {
+  foo();
+}
+
+function empty_block() {
+}
+
+function different_blocks() {
+  if (foo) {
+    let x = foo(); // Noncompliant
+    return x;
+  }
+
+  try {
+    let x = foo(); // Noncompliant
+    return x;
+
+  } catch (e) {
+    let x = foo(); // Noncompliant
+    return x;
+
+  } finally {
+    let x = foo(); // Noncompliant
+    return x;
+  }
+
+
+}
+
+var arrow_function_ok = (a, b) => {
+  return a + b;
+}
+
+var arrow_function_no_block = (a, b) =>  a + b;