Merge branch 'develop'

CHANGES.md

@@ -1,6 +1,13 @@
 Changes
 =======
 
+0.3.2 (2019-10-29)
+
+- Add check ScopeCount
+- Add check UndefinedVariable
+- Fix LocalImportProcedureCheck not properly handling non-locals/definitions
+
+
 0.3.1 (2019-09-22)
 
 - Prevent CPD errors when SYNTAX\_ERROR token is too long

magik-checks/pom.xml

@@ -6,7 +6,7 @@
     <parent>
         <groupId>org.stevenlooman.sw.sonar</groupId>
         <artifactId>sonar-magik</artifactId>
-        <version>0.3.1</version>
+        <version>0.3.2</version>
     </parent>
 
     <artifactId>magik-checks</artifactId>

magik-checks/src/main/java/org/stevenlooman/sw/magik/CheckList.java

@@ -10,10 +10,12 @@
 import org.stevenlooman.sw.magik.checks.LocalImportProcedureCheck;
 import org.stevenlooman.sw.magik.checks.MethodComplexityCheck;
 import org.stevenlooman.sw.magik.checks.MethodDocCheck;
+import org.stevenlooman.sw.magik.checks.ScopeCountCheck;
 import org.stevenlooman.sw.magik.checks.SimplifyIfCheck;
 import org.stevenlooman.sw.magik.checks.SizeZeroEmptyCheck;
 import org.stevenlooman.sw.magik.checks.SyntaxErrorCheck;
 import org.stevenlooman.sw.magik.checks.TrailingWhitespaceCheck;
+import org.stevenlooman.sw.magik.checks.UndefinedVariableCheck;
 import org.stevenlooman.sw.magik.checks.UnusedVariableCheck;
 import org.stevenlooman.sw.magik.checks.VariableNamingCheck;
 import org.stevenlooman.sw.magik.checks.WarnedCallCheck;
@@ -46,10 +48,12 @@ public static List<Class<?>> getChecks() {
       LocalImportProcedureCheck.class,
       MethodComplexityCheck.class,
       MethodDocCheck.class,
+      ScopeCountCheck.class,
       SyntaxErrorCheck.class,
       SimplifyIfCheck.class,
       SizeZeroEmptyCheck.class,
       TrailingWhitespaceCheck.class,
+      UndefinedVariableCheck.class,
       UnusedVariableCheck.class,
       VariableNamingCheck.class,
       WarnedCallCheck.class,

magik-checks/src/main/java/org/stevenlooman/sw/magik/checks/LocalImportProcedureCheck.java

@@ -54,7 +54,8 @@ public void visitNode(AstNode node) {
       for (Scope scope: childScopes) {
         for (ScopeEntry scopeEntry: scope.getScopeEntries()) {
           boolean found = false;
-          if (scopeEntry.getType() == ScopeEntry.Type.IMPORT) {
+          if (scopeEntry.getType() != ScopeEntry.Type.LOCAL
+              && scopeEntry.getType() != ScopeEntry.Type.DEFINITION) {
             continue;
           }
 

magik-checks/src/main/java/org/stevenlooman/sw/magik/checks/ScopeCountCheck.java

@@ -0,0 +1,63 @@
+package org.stevenlooman.sw.magik.checks;
+
+import com.sonar.sslr.api.AstNode;
+import com.sonar.sslr.api.AstNodeType;
+import org.sonar.check.Rule;
+import org.sonar.check.RuleProperty;
+import org.stevenlooman.sw.magik.MagikCheck;
+import org.stevenlooman.sw.magik.analysis.scope.GlobalScope;
+import org.stevenlooman.sw.magik.analysis.scope.Scope;
+import org.stevenlooman.sw.magik.analysis.scope.ScopeEntry;
+import org.stevenlooman.sw.magik.api.MagikGrammar;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+@Rule(key = ScopeCountCheck.CHECK_KEY)
+public class ScopeCountCheck extends MagikCheck {
+
+  public static final String CHECK_KEY = "ScopeCount";
+  private static final String MESSAGE = "Too many variables in scope.";
+  private static final int DEFAULT_MAX_SCOPE_COUNT = 20;
+  @RuleProperty(
+      key = "max scope count",
+      defaultValue = "" + DEFAULT_MAX_SCOPE_COUNT,
+      description = "Maximum number of entries in scope")
+  public int maxScopeCount = DEFAULT_MAX_SCOPE_COUNT;
+
+  @Override
+  public boolean isTemplatedCheck() {
+    return false;
+  }
+
+  @Override
+  public List<AstNodeType> subscribedTo() {
+    return Arrays.asList(
+      MagikGrammar.METHOD_DEFINITION,
+      MagikGrammar.PROC_DEFINITION);
+  }
+
+  /**
+   * Test if there are too many entries in the method/procedure scope.
+   * @param node Node to check.
+   */
+  @Override
+  public void leaveNode(AstNode node) {
+    GlobalScope globalScope = getContext().getGlobalScope();
+    if (globalScope == null) {
+      return;
+    }
+
+    Scope procedureScope = globalScope.getScopeForNode(node);
+    List<ScopeEntry> procedureScopeEntries = procedureScope.getSelfAndDescendantScopes().stream()
+        .map(scope -> scope.getScopeEntries())
+        .flatMap(scopeEntries -> scopeEntries.stream())
+        .filter(scopeEntry -> scopeEntry.getType() != ScopeEntry.Type.IMPORT)
+        .collect(Collectors.toList());
+    if (procedureScopeEntries.size() > maxScopeCount) {
+      addIssue(MESSAGE, node);
+    }
+  }
+
+}

magik-checks/src/main/java/org/stevenlooman/sw/magik/checks/UndefinedVariableCheck.java

@@ -0,0 +1,65 @@
+package org.stevenlooman.sw.magik.checks;
+
+import com.sonar.sslr.api.AstNode;
+import com.sonar.sslr.api.AstNodeType;
+import org.sonar.check.Rule;
+import org.stevenlooman.sw.magik.MagikCheck;
+import org.stevenlooman.sw.magik.analysis.scope.GlobalScope;
+import org.stevenlooman.sw.magik.analysis.scope.Scope;
+import org.stevenlooman.sw.magik.analysis.scope.ScopeEntry;
+import org.stevenlooman.sw.magik.api.MagikGrammar;
+
+import java.util.Arrays;
+import java.util.List;
+
+@Rule(key = UndefinedVariableCheck.CHECK_KEY)
+public class UndefinedVariableCheck extends MagikCheck {
+
+  public static final String CHECK_KEY = "UndefinedVariable";
+  private static final String MESSAGE =
+      "Variable %s is expected to be declared, but used as a global.";
+
+  @Override
+  public boolean isTemplatedCheck() {
+    return false;
+  }
+
+  @Override
+  public List<AstNodeType> subscribedTo() {
+    return Arrays.asList(
+      MagikGrammar.METHOD_DEFINITION,
+      MagikGrammar.PROC_DEFINITION);
+  }
+
+  /**
+   * Test if any global variable in the method/procedure scope has a prefix.
+   * @param node Node to check.
+   */
+  @Override
+  public void leaveNode(AstNode node) {
+    GlobalScope globalScope = getContext().getGlobalScope();
+    if (globalScope == null) {
+      return;
+    }
+
+    Scope procedureScope = globalScope.getScopeForNode(node);
+    procedureScope.getSelfAndDescendantScopes().stream()
+        .map(scope -> scope.getScopeEntries())
+        .flatMap(scopeEntries -> scopeEntries.stream())
+        .filter(scopeEntry -> scopeEntry.getType() == ScopeEntry.Type.GLOBAL)
+        .filter(scopeEntry -> {
+          String identifier = scopeEntry.getIdentifier().toLowerCase();
+          return identifier.startsWith("l_")
+                 || identifier.startsWith("i_")
+                 || identifier.startsWith("p_")
+                 || identifier.startsWith("c_");
+        })
+        .forEach(scopeEntry -> {
+          AstNode scopeEntryNode = scopeEntry.getNode();
+          String identifier = scopeEntry.getIdentifier();
+          String message = String.format(MESSAGE, identifier);
+          addIssue(message, scopeEntryNode);
+        });
+  }
+
+}

magik-checks/src/main/java/org/stevenlooman/sw/magik/checks/VariableNamingCheck.java

@@ -44,7 +44,8 @@ private String stripPrefix(String identifier) {
     String lowered = identifier.toLowerCase();
     if (lowered.startsWith("p_")
         || lowered.startsWith("l_")
-        || lowered.startsWith("i_")) {
+        || lowered.startsWith("i_")
+        || lowered.startsWith("c_")) {
       return identifier.substring(2);
     }
     return identifier;

magik-checks/src/main/resources/org/stevenlooman/sw/sonar/l10n/magik/rules/ScopeCount.html

@@ -0,0 +1 @@
+<p>The number of variables in scope is too high.</p>

magik-checks/src/main/resources/org/stevenlooman/sw/sonar/l10n/magik/rules/ScopeCount.json

@@ -0,0 +1,15 @@
+{
+  "title": "Too many variables in scope",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "30min"
+  },
+  "tags": [
+    "complexity"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "ScopeCount",
+  "sqKey": "scope-count"
+}

magik-checks/src/main/resources/org/stevenlooman/sw/sonar/l10n/magik/rules/UndefinedVariable.html

@@ -0,0 +1,14 @@
+<p>The prefixed variable indicates it should have been declared in the local scope, but was used as a global.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+_method object.do_something()
+  write(l_a)
+_endmethod
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+_method object.do_something()
+  _local l_a &lt;&lt; 10
+  write(l_a)
+_endmethod
+</pre>

magik-checks/src/main/resources/org/stevenlooman/sw/sonar/l10n/magik/rules/UndefinedVariable.json

@@ -0,0 +1,15 @@
+{
+  "title": "Prefixed variable used as a global",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "15min"
+  },
+  "tags": [
+    "error"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "UndefinedVariable",
+  "sqKey": "undefined-variable"
+}

magik-checks/src/test/java/org/stevenlooman/sw/magik/checks/LocalImportProcedureCheckTest.java

@@ -37,7 +37,19 @@ public void testLocalButMeantImport() {
         "  _endproc\n" +
         "_endmethod";
     List<MagikIssue> issues = runCheck(code, check);
-    assertThat(issues).isNotEmpty();
+    assertThat(issues).hasSize(1);
+  }
+
+  @Test
+  public void testMethodProcedureParameter() {
+    MagikCheck check = new LocalImportProcedureCheck();
+    String code =
+        "_method a.a(p_a)\n" +
+        "  _proc(p_a)\n" +
+        "  _endproc\n" +
+        "_endmethod";
+    List<MagikIssue> issues = runCheck(code, check);
+    assertThat(issues).isEmpty();
   }
 
   @Test

magik-checks/src/test/java/org/stevenlooman/sw/magik/checks/ScopeCountCheckTest.java

@@ -0,0 +1,49 @@
+package org.stevenlooman.sw.magik.checks;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+import java.util.List;
+
+import org.junit.Test;
+import org.stevenlooman.sw.magik.MagikIssue;
+
+public class ScopeCountCheckTest extends MagikCheckTestBase {
+
+  @Test
+  public void testTooManyScopeEntries() {
+    ScopeCountCheck check = new ScopeCountCheck();
+    check.maxScopeCount = 1;
+    String code =
+        "_method a.b\n" +
+        "\t_local l_a, l_b\n" +
+        "_endmethod";
+    List<MagikIssue> issues = runCheck(code, check);
+    assertThat(issues).hasSize(1);
+  }
+
+  @Test
+  public void testOk() {
+    ScopeCountCheck check = new ScopeCountCheck();
+    check.maxScopeCount = 10;
+    String code =
+        "_method a.b\n" +
+        "\t_local l_a, l_b\n" +
+        "_endmethod";
+    List<MagikIssue> issues = runCheck(code, check);
+    assertThat(issues).isEmpty();
+  }
+
+  @Test
+  public void testTooManyScopeEntriesGlobals() {
+    ScopeCountCheck check = new ScopeCountCheck();
+    check.maxScopeCount = 1;
+    String code =
+        "_method a.b\n" +
+        "\t_global a, b\n" +
+        "_endmethod";
+    List<MagikIssue> issues = runCheck(code, check);
+    assertThat(issues).hasSize(1);
+  }
+
+
+}