Fix the remaining audit bugs and close the test gaps

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/languageserver/ModelManager.java

@@ -141,15 +141,26 @@ class Changes {
         private static final Changes EMPTY = new Changes(HashSet.empty(), HashSet.empty());
         private final HashSet<WFile> affectedFiles;
         private final HashSet<String> affectedPackageNames;
+        private final boolean jassNamesChanged;
 
         public Changes(Iterable<WFile> affectedFiles, Iterable<String> affectedPackageNames) {
+            this(affectedFiles, affectedPackageNames, false);
+        }
+
+        /**
+         * @param jassNamesChanged whether a unit which declared Jass names was replaced or removed (see
+         *                         {@link #isJassNamesChanged()})
+         */
+        public Changes(Iterable<WFile> affectedFiles, Iterable<String> affectedPackageNames, boolean jassNamesChanged) {
             this.affectedFiles = HashSet.ofAll(affectedFiles);
             this.affectedPackageNames = HashSet.ofAll(affectedPackageNames);
+            this.jassNamesChanged = jassNamesChanged;
         }
 
         public Changes(Stream<WFile> affectedFiles, Stream<String> affectedPackageNames) {
             this.affectedFiles = HashSet.ofAll(affectedFiles);
             this.affectedPackageNames = HashSet.ofAll(affectedPackageNames);
+            this.jassNamesChanged = false;
         }
 
         public static Changes empty() {
@@ -164,17 +175,27 @@ public HashSet<String> getAffectedPackageNames() {
             return affectedPackageNames;
         }
 
+        /**
+         * Whether a unit which declared Jass names (a .j file, or Jass outside of the packages of a .wurst or .jurst
+         * file) was replaced or removed. Every package sees those names without an import, so every unit has to be
+         * checked again, and a removed unit is not in the model any more to tell.
+         */
+        public boolean isJassNamesChanged() {
+            return jassNamesChanged;
+        }
+
         public Changes mergeWith(Changes affected) {
             HashSet<WFile> newF = affectedFiles.addAll(affected.affectedFiles);
             HashSet<String> newP = affectedPackageNames.addAll(affected.affectedPackageNames);
-            if (newF == affectedFiles && newP == affectedPackageNames) {
+            boolean newJ = jassNamesChanged || affected.jassNamesChanged;
+            if (newF == affectedFiles && newP == affectedPackageNames && newJ == jassNamesChanged) {
                 return this;
             }
-            return new Changes(newF, newP);
+            return new Changes(newF, newP, newJ);
         }
 
         public boolean isEmpty() {
-            return affectedFiles.isEmpty() && affectedPackageNames.isEmpty();
+            return affectedFiles.isEmpty() && affectedPackageNames.isEmpty() && !jassNamesChanged;
         }
     }
 

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/languageserver/ModelManagerImpl.java

@@ -136,6 +136,7 @@ private List<CompilationUnit> getJassdocCUs(Path jassdoc, WurstGui gui) {
     public Changes removeCompilationUnit(WFile resource) {
         WurstModel model2 = model;
         List<CompilationUnit> toRemove = new ArrayList<>();
+        boolean jassNamesRemoved = false;
         if (model2 != null) {
             for (CompilationUnit compilationUnit : model2) {
                 if (wFile(compilationUnit).equals(resource)) {
@@ -148,16 +149,18 @@ public Changes removeCompilationUnit(WFile resource) {
                 .flatMap(cu -> cu.getPackages().stream())
                 .map(WPackage::getName)
                 .collect(Collectors.toSet());
-            Set<CompilationUnit> dependents = toRemove.isEmpty() ? Collections.emptySet()
-                : calculateCUsToUpdate(Collections.emptyList(), removedPackages, model2);
-            synchronized (modelLock) {
-                model2.removeAll(toRemove);
-                uncheckedUnits.removeAll(toRemove);
+            jassNamesRemoved = toRemove.stream().anyMatch(ModelManagerImpl::declaresJassNames);
+            if (!toRemove.isEmpty()) {
+                Set<CompilationUnit> dependents =
+                    calculateCUsToUpdate(Collections.emptyList(), removedPackages, jassNamesRemoved, model2);
                 dependents.removeAll(toRemove);
-                uncheckedUnits.addAll(dependents);
-                if (toRemove.stream().anyMatch(cu -> cu.getCuInfo().getFile().endsWith(".j"))) {
-                    // Jass names are visible everywhere
-                    uncheckedUnits.addAll(model2);
+                // what used the removed units resolves its names again, as after a replacement: a check of the
+                // whole model before the reconciliation must not find the bindings into the removed units
+                clearCompilationUnits(dependents);
+                synchronized (modelLock) {
+                    model2.removeAll(toRemove);
+                    uncheckedUnits.removeAll(toRemove);
+                    uncheckedUnits.addAll(dependents);
                 }
             }
         }
@@ -173,7 +176,8 @@ public Changes removeCompilationUnit(WFile resource) {
             toRemove.stream()
                 .flatMap(cu -> cu.getPackages().stream())
                 .map(WPackage::getName)
-                .collect(Collectors.toList())
+                .collect(Collectors.toList()),
+            jassNamesRemoved
         );
     }
 
@@ -743,7 +747,8 @@ private void updateModel(CompilationUnit cu, WurstGui gui) {
                     // get old provided packages:
                     Set<String> oldPackages = providedPackages(c);
                     reconciliations++;
-                    Set<CompilationUnit> mustUpdate = calculateCUsToUpdate(Collections.singletonList(cu), oldPackages, model2);
+                    Set<CompilationUnit> mustUpdate = calculateCUsToUpdate(Collections.singletonList(cu), oldPackages,
+                        declaresJassNames(c), model2);
 
                     GlobalCaches.clearLookupCacheFor(Collections.singletonList(c));
                     clearCompilationUnits(mustUpdate);
@@ -769,10 +774,16 @@ private void updateModel(CompilationUnit cu, WurstGui gui) {
                         uncheckedUnits.add(cu);
                     }
                 }
-                if (!everyUnitUnchecked) {
-                    // what imports the new packages (or sees the new Jass names) has to be checked again
+                if (everyUnitUnchecked) {
+                    // the packages and names of the model include the new unit
+                    model2.clearAttributesLocal();
+                } else {
+                    // what imports the new packages (or sees the new Jass names) has to be checked again, and resolves
+                    // its names again, as after a replacement
                     reconciliations++;
-                    Set<CompilationUnit> mustUpdate = calculateCUsToUpdate(Collections.singletonList(cu), Collections.emptySet(), model2);
+                    Set<CompilationUnit> mustUpdate = calculateCUsToUpdate(Collections.singletonList(cu),
+                        Collections.emptySet(), false, model2);
+                    clearCompilationUnits(mustUpdate);
                     synchronized (modelLock) {
                         uncheckedUnits.addAll(mustUpdate);
                     }
@@ -887,8 +898,9 @@ public Changes syncCompilationUnitContent(WFile filename, String contents) {
             return Changes.empty();
         }
         Set<String> oldPackages = declaredPackages(filename);
+        boolean oldJassNames = declaredJassNames(filename);
         replaceCompilationUnit(filename, contents, false);
-        return new Changes(io.vavr.collection.HashSet.of(filename), oldPackages);
+        return new Changes(io.vavr.collection.HashSet.of(filename), oldPackages, oldJassNames);
     }
 
     private Set<String> declaredPackages(WFile f) {
@@ -909,6 +921,21 @@ private Set<String> declaredPackages(WFile f) {
         return Collections.emptySet();
     }
 
+    /** Whether the unit of the file declares Jass names, see {@link #declaresJassNames(CompilationUnit)}. */
+    private boolean declaredJassNames(WFile f) {
+        CompilationUnit cu = getCompilationUnit(f);
+        return cu != null && declaresJassNames(cu);
+    }
+
+    /**
+     * Whether the unit declares Jass names: a .j file, or Jass declared outside of the packages of a .wurst or .jurst
+     * file. Every package sees those names without an import (the scope of the model holds the Jass declarations of
+     * every unit), so a change to them concerns every unit of the model.
+     */
+    private static boolean declaresJassNames(CompilationUnit cu) {
+        return cu.getCuInfo().getFile().endsWith(".j") || !cu.getJassDecls().isEmpty();
+    }
+
     @Override
     public CompilationUnit replaceCompilationUnitContent(WFile filename, String contents, boolean reportErrors) {
         return replaceCompilationUnit(filename, contents, reportErrors);
@@ -939,11 +966,12 @@ public Changes syncCompilationUnit(WFile f) {
             return Changes.empty();
         }
         Set<String> oldPackages = declaredPackages(f);
+        boolean oldJassNames = declaredJassNames(f);
         replaceCompilationUnit(f, contents, true);
         WLogger.debug("replaced file " + f);
         WurstGui gui = new WurstGuiLogger();
-        doTypeCheckPartial(gui, ImmutableList.of(f), oldPackages);
-        return new Changes(io.vavr.collection.HashSet.of(f), oldPackages);
+        doTypeCheckPartial(gui, ImmutableList.of(f), oldPackages, oldJassNames);
+        return new Changes(io.vavr.collection.HashSet.of(f), oldPackages, oldJassNames);
     }
 
     private @Nullable String readCompilationUnitContents(WFile filename, boolean preferOpenBuffer) throws IOException {
@@ -1116,7 +1144,8 @@ public void onCompilationResult(Consumer<PublishDiagnosticsParams> f) {
         onCompilationResultListeners.add(f);
     }
 
-    private void doTypeCheckPartial(WurstGui gui, List<WFile> toCheckFilenames, Set<String> oldPackages) {
+    private void doTypeCheckPartial(WurstGui gui, List<WFile> toCheckFilenames, Set<String> oldPackages,
+                                    boolean oldJassNames) {
         WLogger.debug("do typecheck partial of " + toCheckFilenames);
         WurstCompilerJassImpl comp = getCompiler(gui);
         List<CompilationUnit> toCheck = getCompilationUnits(toCheckFilenames);
@@ -1126,7 +1155,7 @@ private void doTypeCheckPartial(WurstGui gui, List<WFile> toCheckFilenames, Set<
             return;
         }
 
-        Collection<CompilationUnit> toCheckRec = calculateCUsToUpdate(toCheck, oldPackages, model2);
+        Collection<CompilationUnit> toCheckRec = calculateCUsToUpdate(toCheck, oldPackages, oldJassNames, model2);
 
         partialTypecheck(model2, toCheckRec, gui, comp);
     }
@@ -1144,14 +1173,11 @@ public void reconcile(Changes changes) {
             }
         }
         Set<String> oldPackageNames = changes.getAffectedPackageNames().toJavaSet();
-        Collection<CompilationUnit> toCheckRec = calculateCUsToUpdate(toCheck1, oldPackageNames, model2);
-        boolean jassFileChanged = changes.getAffectedFiles().toJavaSet().stream()
+        // A removed or replaced unit is no longer in the model, so calculateCUsToUpdate cannot see its Jass names
+        // among the changed compilation units.
+        boolean jassNamesChanged = changes.isJassNamesChanged() || changes.getAffectedFiles().toJavaSet().stream()
             .anyMatch(file -> file.getUriString().endsWith(".j"));
-        if (jassFileChanged) {
-            // A removed Jass CU is no longer in the model, so calculateCUsToUpdate
-            // cannot see it among the changed compilation units.
-            toCheckRec.addAll(model2);
-        }
+        Collection<CompilationUnit> toCheckRec = calculateCUsToUpdate(toCheck1, oldPackageNames, jassNamesChanged, model2);
         WurstGui gui = new WurstGuiLogger();
         WurstCompilerJassImpl comp = getCompiler(gui);
         partialTypecheck(model2, toCheckRec, gui, comp);
@@ -1204,23 +1230,18 @@ private Collection<CompilationUnit> withUncheckedUnits(WurstModel model2, Collec
      *
      * @param changed the set of compilation units that were changed
      * @param oldPackages packages that were provided before the update (which might have been removed now)
+     * @param oldJassNames whether the units before the update (which might have been removed now) declared Jass names
      * @param model the complete AST
      * @return the set of compilation units that might be affected by the changes, including the changed compilation units
      */
-    private Set<CompilationUnit> calculateCUsToUpdate(Collection<CompilationUnit> changed, Set<String> oldPackages, WurstModel model) {
+    private Set<CompilationUnit> calculateCUsToUpdate(Collection<CompilationUnit> changed, Set<String> oldPackages,
+                                                      boolean oldJassNames, WurstModel model) {
 
         Set<CompilationUnit> result = new TreeSet<>(Comparator.comparing(cu -> cu.getCuInfo().getFile()));
         result.addAll(changed);
 
-        boolean b = false;
-        for (CompilationUnit compilationUnit : changed) {
-            if (compilationUnit.getCuInfo().getFile().endsWith(".j")) {
-                b = true;
-                break;
-            }
-        }
-        if (b) {
-            // when plain Jass files are changed, everything must be checked again:
+        if (oldJassNames || changed.stream().anyMatch(ModelManagerImpl::declaresJassNames)) {
+            // Jass names are visible everywhere, so everything must be checked again:
             result.addAll(model);
             return result;
         }

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/ControlFlowGraph.java

@@ -4,8 +4,11 @@
 import de.peeeq.wurstscript.jassIm.*;
 import it.unimi.dsi.fastutil.objects.ObjectArrayList;
 import it.unimi.dsi.fastutil.objects.Reference2ObjectOpenHashMap;
+import it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet;
 import org.eclipse.jdt.annotation.Nullable;
 
+import java.util.ArrayDeque;
+import java.util.ArrayList;
 import java.util.List;
 
 public class ControlFlowGraph {
@@ -36,13 +39,61 @@ public static final class Node {
     private final Reference2ObjectOpenHashMap<ImLoop, Node> loopEnd    = new Reference2ObjectOpenHashMap<>();
     private final Reference2ObjectOpenHashMap<ImVarargLoop, Node> varargLoopEnd = new Reference2ObjectOpenHashMap<>();
     private final ObjectArrayList<Node> nodeList = new ObjectArrayList<>();
+    private final ImStmts body;
 
     public ControlFlowGraph(ImStmts stmts) {
+        this.body = stmts;
         // a light hint helps the first growth step avoid rehash
         nodes.trim(0);
         buildCfg(stmts);
     }
 
+    /**
+     * The statements which no path from the first statement reaches, without the statements inside them: what follows
+     * a return, a loop which has no exitwhen of its own, or an if neither branch of which comes to its end.
+     */
+    public List<ImStmt> unreachableStatements() {
+        if (body.isEmpty()) {
+            return List.of();
+        }
+        ReferenceOpenHashSet<Node> reached = new ReferenceOpenHashSet<>(nodeList.size());
+        ArrayDeque<Node> work = new ArrayDeque<>();
+        Node entry = getNode(body.get(0));
+        reached.add(entry);
+        work.add(entry);
+        while (!work.isEmpty()) {
+            ObjectArrayList<Node> successors = work.poll().successors;
+            for (int i = 0; i < successors.size(); i++) {
+                Node successor = successors.get(i);
+                if (reached.add(successor)) {
+                    work.add(successor);
+                }
+            }
+        }
+        if (reached.size() == nodeList.size()) {
+            return List.of();
+        }
+        List<ImStmt> result = new ArrayList<>();
+        collectUnreachable(body, reached, result);
+        return result;
+    }
+
+    private void collectUnreachable(ImStmts stmts, ReferenceOpenHashSet<Node> reached, List<ImStmt> result) {
+        for (int i = 0; i < stmts.size(); i++) {
+            ImStmt s = stmts.get(i);
+            if (!reached.contains(nodes.get(s))) {
+                result.add(s);
+            } else if (s instanceof ImIf imIf) {
+                collectUnreachable(imIf.getThenBlock(), reached, result);
+                collectUnreachable(imIf.getElseBlock(), reached, result);
+            } else if (s instanceof ImLoop imLoop) {
+                collectUnreachable(imLoop.getBody(), reached, result);
+            } else if (s instanceof ImVarargLoop imVarargLoop) {
+                collectUnreachable(imVarargLoop.getBody(), reached, result);
+            }
+        }
+    }
+
     private void buildCfg(ImStmts stmts) {
         final int n = stmts.size();
         for (int i = 0; i < n; i++) {