Warn for existential variables in head pattern referred only once

#265

Change-Id: Ie022bbfb0b1829bc462bf88f44f65009464de096
(cherry picked from commit 413eb9409ab556f22293d28588b39f46d8449718)

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRQuery.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRQuery.java
index 7bee6eae06b08a481652df67e5009d76629dfde4..6b01d99531f9d30db25fe0277e0f98d435632729 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRQuery.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRQuery.java
@@ -10,8 +10,11 @@ import org.simantics.scl.compiler.elaboration.contexts.SimplificationContext;
 import org.simantics.scl.compiler.elaboration.contexts.TranslationContext;
 import org.simantics.scl.compiler.elaboration.contexts.TypingContext;
 import org.simantics.scl.compiler.elaboration.expressions.Expression;
+import org.simantics.scl.compiler.elaboration.expressions.ExpressionVisitor;
 import org.simantics.scl.compiler.elaboration.expressions.Variable;
 import org.simantics.scl.compiler.elaboration.expressions.printing.ExpressionToStringVisitor;
+import org.simantics.scl.compiler.elaboration.expressions.visitors.ForVariablesUsesVisitor;
+import org.simantics.scl.compiler.elaboration.expressions.visitors.StandardExpressionVisitor;
 import org.simantics.scl.compiler.errors.Locations;
 import org.simantics.scl.compiler.internal.parsing.Symbol;
 
@@ -92,4 +95,16 @@ public class CHRQuery extends Symbol {
             newLiterals[i] = literals[i].replace(context);
         return new CHRQuery(location, newLiterals);
     }
+    
+    public void accept(ExpressionVisitor visitor) {
+        for(CHRLiteral literal : literals) {
+            if(literal == null || literal.parameters == null)
+                continue; // FIXME why this happens?
+            for(Expression parameter : literal.parameters) {
+                if(parameter == null)
+                    continue; // FIXME why this happens?
+                parameter.accept(visitor);
+            }
+        }
+    }
 }

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRRule.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRRule.java
index 78224ebb0d75d876f0da2528a4ab8fba56a33129..e698d72c618ee79fca251e8689ae0267f057e5a4 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRRule.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/chr/CHRRule.java
@@ -1,6 +1,7 @@
 package org.simantics.scl.compiler.elaboration.chr;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 
 import org.simantics.scl.compiler.compilation.CompilationContext;
 import org.simantics.scl.compiler.elaboration.chr.plan.CHRSearchPlan;
@@ -9,9 +10,13 @@ import org.simantics.scl.compiler.elaboration.chr.relations.CHRConstraint;
 import org.simantics.scl.compiler.elaboration.contexts.SimplificationContext;
 import org.simantics.scl.compiler.elaboration.contexts.TranslationContext;
 import org.simantics.scl.compiler.elaboration.contexts.TypingContext;
+import org.simantics.scl.compiler.elaboration.expressions.EAsPattern;
 import org.simantics.scl.compiler.elaboration.expressions.EVariable;
+import org.simantics.scl.compiler.elaboration.expressions.Expression;
+import org.simantics.scl.compiler.elaboration.expressions.ExpressionVisitor;
 import org.simantics.scl.compiler.elaboration.expressions.Variable;
 import org.simantics.scl.compiler.elaboration.expressions.printing.ExpressionToStringVisitor;
+import org.simantics.scl.compiler.elaboration.expressions.visitors.StandardExpressionVisitor;
 import org.simantics.scl.compiler.errors.Locations;
 import org.simantics.scl.compiler.internal.parsing.Symbol;
 import org.simantics.scl.compiler.types.Types;
@@ -54,6 +59,52 @@ public class CHRRule extends Symbol {
         context.disallowNewExistentials();
         body.resolve(context);
         existentialVariables = context.popExistentialFrame();
+        
+        warnForExistentialsUsedOnlyOnce(context);
+    }
+
+    private static final Object NEVER_USED = new Object();
+    
+    private void warnForExistentialsUsedOnlyOnce(TranslationContext context) {
+        // Initialize the hash map
+        HashMap<Variable, Object> usageCount = new HashMap<>(existentialVariables.length);
+        for(Variable var : existentialVariables)
+            if(!var.getName().equals("_"))
+                usageCount.put(var, NEVER_USED);
+    
+        // Collect variable uses
+        ExpressionVisitor visitor = new StandardExpressionVisitor() {
+            private void handle(Expression expression, Variable variable) {
+                Object object = usageCount.remove(variable);
+                if(object == NEVER_USED)
+                    usageCount.put(variable, expression);
+            }
+            @Override
+            public void visit(EVariable expression) {
+                if(expression.variable != null)
+                    handle(expression, expression.variable);
+            }
+            @Override
+            public void visit(EAsPattern expression) {
+                expression.pattern.accept(this);
+                handle(expression, expression.var);
+            }
+        };
+        head.accept(visitor);
+        body.accept(visitor);
+        
+        // Report as warnings
+        usageCount.forEach((variable, expression_) -> {
+            if(!(expression_ instanceof Expression))
+                return; // Should never happen
+            Expression expression = (Expression)expression_;
+            if(context.isExpandedFromWildcard(expression))
+                return;
+            
+            context.getErrorLog().logWarning(expression.location,
+                    "Existential variable " + variable.getName() + " is referred only once. Replace by _ if this is a wildcard.");
+        });
+        
     }
 
     public void checkType(TypingContext context) {

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/contexts/TranslationContext.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/contexts/TranslationContext.java
index d8346f41f5e889fbc66437a1f9be10a1a3ad803c..e0c256599195097c1e0a5d3650334deebebf4265 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/contexts/TranslationContext.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/contexts/TranslationContext.java
@@ -76,6 +76,8 @@ public class TranslationContext extends TypeTranslationContext implements Enviro
     TIntArrayList chrConstraintFrames = new TIntArrayList();
     ArrayList<CHRConstraintEntry> chrConstraintEntries = new ArrayList<CHRConstraintEntry>();
     
+    private THashSet<Expression> expandedFromWildcard;
+    
     public CHRRuleset currentRuleset;
     
     public ModuleDebugInfo moduleDebugInfo;
@@ -141,7 +143,7 @@ public class TranslationContext extends TypeTranslationContext implements Enviro
             variable = new Variable(name);
             variables.put(name, variable);
             existentialFrame.variables.add(name);
-            return new EVariable(variable);
+            return new EVariable(location, variable);
         }
         case '_': {
             if(name.length()==1) {
@@ -583,7 +585,29 @@ public class TranslationContext extends TypeTranslationContext implements Enviro
         return Environments.getRuleset(environment, name);
     }
 
+    /**
+     * Tells that new existential variables are no longer allowed in this context.
+     */
     public void disallowNewExistentials() {
         getCurrentExistentialFrame().disallowNewExistentials = true;
     }
+
+    /**
+     * Marks that the expression is a result of expanding .. wildcard pattern in records.
+     */
+    public void addExpandedFromWildcard(Expression expression) {
+        if(expandedFromWildcard == null)
+            expandedFromWildcard = new THashSet<>();
+        expandedFromWildcard.add(expression);
+    }
+    
+    /**
+     * Asks if the expression is a result of expanding .. wildcard pattern in records.
+     */
+    public boolean isExpandedFromWildcard(Expression expression) {
+        if(expandedFromWildcard == null)
+            return false;
+        else
+            return expandedFromWildcard.contains(expression);
+    }
 }

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/ERecord.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/ERecord.java
index 47ef205df17cbce0d9ff1d74dd8f1154adc1bcfa..cb34d0fb3ea0394035bf61800430465e3d9b4376 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/ERecord.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/ERecord.java
@@ -150,7 +150,9 @@ public class ERecord extends ASTExpression {
                 String variableName = fieldNames[i];
                 if(chrLiteral)
                     variableName = "?" + variableName;
-                parameters[i] = new EVar(wildcardField.location, variableName);
+                EVar expandedVar = new EVar(wildcardField.location, variableName); 
+                parameters[i] = expandedVar;
+                context.addExpandedFromWildcard(expandedVar);
             }
         }
         if(!recordMap.isEmpty()) {

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/EVar.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/EVar.java
index d78fa8565abf2fa3c8147baf797bd0a10f7920b1..2e4809a0c871160885200f84634db12beb47f769 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/EVar.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/EVar.java
@@ -49,7 +49,10 @@ public class EVar extends ASTExpression {
 
     @Override
     public Expression resolve(TranslationContext context) {
-        return context.resolveVariable(location, name);
+        Expression resolved = context.resolveVariable(location, name);
+        if(context.isExpandedFromWildcard(this))
+            context.addExpandedFromWildcard(resolved);
+        return resolved;
     }
     
     @Override
@@ -59,7 +62,10 @@ public class EVar extends ASTExpression {
     
     @Override
     public Expression resolveAsPattern(TranslationContext context) {
-        return context.resolvePattern(this);
+        Expression resolved = context.resolvePattern(this);
+        if(context.isExpandedFromWildcard(this))
+            context.addExpandedFromWildcard(resolved);
+        return resolved;
     }
     
     @Override

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/VariableProcedure.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/VariableProcedure.java
index 44e9b6ae7264ae5e02581d3cea3b4c52884820db..af50692fa4ab8a8ec8244e6ef2f3bd90c3615f90 100644 (file)
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/VariableProcedure.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/elaboration/expressions/VariableProcedure.java
@@ -1,5 +1,6 @@
 package org.simantics.scl.compiler.elaboration.expressions;
 
+@FunctionalInterface
 public interface VariableProcedure {
     void execute(long location, Variable variable);
 }

diff --git a/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR4.scl b/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR4.scl
index dc0714973abbbd9da901fe4d91df7e94442bcd0e..90b82e79a016d8c8db7b6eea99e19cf09a834dc0 100644 (file)
--- a/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR4.scl
+++ b/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR4.scl
@@ -3,7 +3,9 @@ main = ()
     when ?x <- ?y
     then True
 --
+3:10-3:12: Existential variable ?x is referred only once. Replace by _ if this is a wildcard.
 3:10-3:18: Cannot solve the query.
+3:16-3:18: Existential variable ?y is referred only once. Replace by _ if this is a wildcard.
 --
 import "Prelude"
 

diff --git a/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR8.scl b/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR8.scl
index dd915ebb5d10b971f5ec7cccc8cc3d7440a16cca..6929a0b9291a441cf31fd390d419d33b5d4f1308 100644 (file)
--- a/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR8.scl
+++ b/tests/org.simantics.scl.compiler.tests/src/org/simantics/scl/compiler/tests/scl/CHR8.scl
@@ -5,4 +5,5 @@ main = ()
   where
     X ?x => Y ?y
 --
+6:7-6:9: Existential variable ?x is referred only once. Replace by _ if this is a wildcard.
 6:15-6:17: New existential variables can be defined only in queries.
\ No newline at end of file