From 8dc77dd34f4359c9e27381b6a1c9a4ef456929ff Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Thu, 10 Jul 2025 10:57:24 +0200 Subject: [PATCH 01/15] Bump Nextflow version Signed-off-by: ErikDanielsson --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 041470e..ad6c3f7 100644 --- a/build.gradle +++ b/build.gradle @@ -27,7 +27,7 @@ test { } dependencies { - implementation 'io.nextflow:nf-lang:25.05.0-edge' + implementation 'io.nextflow:nf-lang:25.06.0-edge' implementation 'org.apache.groovy:groovy:4.0.27' implementation 'org.apache.groovy:groovy-json:4.0.27' implementation 'org.eclipse.lsp4j:org.eclipse.lsp4j:0.23.0' From f0e786395e005c4ca5cd4c2086752de68630c9b6 Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Mon, 14 Jul 2025 10:33:20 +0200 Subject: [PATCH 02/15] First working version --- .../services/script/dag/DataflowVisitor.java | 453 +++++++++++++----- .../services/script/dag/MermaidRenderer.java | 115 +++-- 2 files changed, 426 insertions(+), 142 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index 8b8c508..59d6bbb 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -1,17 +1,15 @@ /* * Copyright 2024-2025, Seqera Labs * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. */ package nextflow.lsp.services.script.dag; @@ -28,6 +26,7 @@ import java.util.Stack; import java.util.stream.Collectors; +import groovy.lang.Tuple2; import groovy.lang.Tuple3; import nextflow.lsp.ast.LanguageServerASTUtils; import nextflow.lsp.services.script.ScriptAstCache; @@ -47,26 +46,70 @@ import static nextflow.script.ast.ASTUtils.*; import static org.codehaus.groovy.ast.tools.GeneralUtils.*; - /** * * @author Ben Sherman */ + +class Variable { + private int defintionDepth; + private Set activeInstances; + + public Variable(Node activeInstance, int defintionDepth) { + this.activeInstances = new HashSet<>(); + activeInstances.add(activeInstance); + this.defintionDepth = defintionDepth; + } + + private Variable(Set activeInstances, int defintionDepth) { + this.activeInstances = new HashSet<>(activeInstances); + this.defintionDepth = defintionDepth; + } + + public Variable shallowCopy() { + return new Variable(activeInstances, defintionDepth); + } + + public boolean isLocal(int currDepth) { + return defintionDepth == currDepth; + } + + public Set getActiveInstances() { + return activeInstances; + } + + public void setActiveInstance(Node dn) { + activeInstances.clear(); + activeInstances.add(dn); + } + + public Variable union(Variable other) { + var unionInstances = new HashSet<>(activeInstances); + unionInstances.addAll(other.getActiveInstances()); + return new Variable(unionInstances, defintionDepth); + } +} + public class DataflowVisitor extends ScriptVisitorSupport { private SourceUnit sourceUnit; private ScriptAstCache ast; - private Map graphs = new HashMap<>(); + private Map graphs = new HashMap<>(); private Stack> stackPreds = new Stack<>(); + private Stack> conditionalScopes = new Stack<>(); + + private int currentDepth = 0; + public DataflowVisitor(SourceUnit sourceUnit, ScriptAstCache ast) { this.sourceUnit = sourceUnit; this.ast = ast; stackPreds.add(new HashSet<>()); + conditionalScopes.push(new HashMap<>()); } @Override @@ -94,6 +137,80 @@ public Graph getGraph(String name) { private boolean inEntry; + public Set getSymbolConditionalScope(String name) { + // get a variable node from the name table + var scope = conditionalScopes.peek(); + if( scope.containsKey(name) ) { + return scope.get(name).getActiveInstances(); + } else { + return null; + } + } + + public void putSymbolConditionalScope(String name, Node dn) { + // Put a symbol into the currently active symbol table + var scope = conditionalScopes.peek(); + if( scope.containsKey(name) ) { + // We have reassinged the variable, which means that all previous + // definitions of it (in this scope) are dead. We can reuse the set + // though + Variable variable = scope.get(name); + variable.setActiveInstance(dn); + } else { + // The symbols in not present from before so create an empty set + Variable variable = new Variable(dn, currentDepth); + scope.put(name, variable); + } + } + + public void pushConditionalScope(Map previousSymbols) { + // We want to add all symbols in the current scope + // since they always enter both branches + var newScope = new HashMap(); + for( Map.Entry entry : previousSymbols.entrySet() ) { + newScope.put(entry.getKey(), entry.getValue().shallowCopy()); + } + conditionalScopes.push(newScope); + currentDepth += 1; + } + + public Map mergeConditionalScopes(Map ifSymbols, + Map elseSymbols) { + // Merge the set nodes corresponding to active symbols for the two + // scopes + // NOTE: Since we can define local variables with `def` we should have a + // flag + // which tells us + // if this symbol is locally defined in this scope or defined outside + // the scope. + var merged = new HashMap(); + // Add all keys from ifSymbols + for( Map.Entry entry : ifSymbols.entrySet() ) { + String key = entry.getKey(); + Variable ifVariable = entry.getValue(); + if( elseSymbols.containsKey(key) ) { + merged.put(key, ifVariable.union(elseSymbols.get(key))); + } else { + merged.put(key, ifVariable.shallowCopy()); + } + } + + // Add remaining keys from elseSymbols (already handled common ones) + for( Map.Entry entry : elseSymbols.entrySet() ) { + String key = entry.getKey(); + if( !merged.containsKey(key) ) { + merged.put(key, entry.getValue().shallowCopy()); + } + } + + return merged; + } + + public Map popConditionalScope() { + currentDepth -= 1; + return conditionalScopes.pop(); + } + @Override public void visitWorkflow(WorkflowNode node) { current = new Graph(); @@ -110,39 +227,42 @@ public void visitWorkflow(WorkflowNode node) { current = null; } - private void visitWorkflowTakes(WorkflowNode node, Map result) { + private void visitWorkflowTakes(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.takes) ) { var name = asVarX(stmt).getName(); var dn = addNode(name, Node.Type.NAME, stmt); - current.putSymbol(name, dn); + putSymbolConditionalScope(name, dn); result.put(name, dn); } } - private void visitWorkflowEmits(WorkflowNode node, Map result) { + private void visitWorkflowEmits(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.emits) ) { var emit = ((ExpressionStatement) stmt).getExpression(); String name; if( emit instanceof VariableExpression ve ) { name = ve.getName(); - } - else if( emit instanceof AssignmentExpression assign ) { + } else if( emit instanceof AssignmentExpression assign ) { var target = (VariableExpression) assign.getLeftExpression(); name = target.getName(); visit(emit); - } - else { + } else { name = "$out"; visit(new AssignmentExpression(varX(name), emit)); } - var dn = current.getSymbol(name); - if( dn == null ) + var dns = getSymbolConditionalScope(name); + if( dns == null ) { System.err.println("missing emit: " + name); - result.put(name, dn); + result.put(name, null); + } else if( dns.size() > 1 ) { + System.err.println("two many emits: " + name); + } else { + result.put(name, dns.iterator().next()); + } } } - private void visitWorkflowPublishers(WorkflowNode node, Map result) { + private void visitWorkflowPublishers(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.publishers) ) { var es = (ExpressionStatement) stmt; var publisher = (BinaryExpression) es.getExpression(); @@ -150,7 +270,10 @@ private void visitWorkflowPublishers(WorkflowNode node, Map result) var source = publisher.getRightExpression(); visit(new AssignmentExpression(target, source)); var name = target.getName(); - result.put(name, current.getSymbol(name)); + var dns = getSymbolConditionalScope(name); + if( dns.size() == 1 ) { + result.put(name, dns.iterator().next()); + } } } @@ -173,7 +296,8 @@ public void visitMethodCallExpression(MethodCallExpression node) { var defNode = (MethodNode) node.getNodeMetaData(ASTNodeMarker.METHOD_TARGET); if( defNode instanceof WorkflowNode || defNode instanceof ProcessNode ) { var preds = visitWithPreds(node.getArguments()); - current.putSymbol(name, addNode(name, Node.Type.OPERATOR, defNode, preds)); + var dn = addNode(name, Node.Type.OPERATOR, defNode, preds); + putSymbolConditionalScope(name, dn); return; } @@ -194,27 +318,82 @@ public void visitBinaryExpression(BinaryExpression node) { super.visitBinaryExpression(node); } - private void visitAssignment(BinaryExpression node) { - var preds = visitWithPreds(node.getRightExpression()); - var targets = getAssignmentTargets(node.getLeftExpression()); - for( var name : targets ) { - var dn = addNode(name, Node.Type.NAME, null, preds); - current.putSymbol(name, dn); + @Override + public void visitIfElse(IfStatement node) { + // First visit the conditional + var preds = visitWithPreds(node.getBooleanExpression()); + var conditionalDn = addNode("", Node.Type.CONDITIONAL, node, preds); + + // Then construct new symbol tables for each of the branches + // Get the symbol table associated with the currently available global + // variables + var precedingScope = popConditionalScope(); + // Construct the scope for the if branch + pushConditionalScope(precedingScope); + // Push a new subgraph to keep track of the if branch + current.pushSubgraph(); + current.putSubgraphPred(conditionalDn); + visitWithPreds(node.getIfBlock()); + var ifScope = popConditionalScope(); + current.popSubgraph(); + if( !node.getElseBlock().isEmpty() ) { + // We push a new symbol table to keep track of the + // symbols in the else branch. + pushConditionalScope(precedingScope); + // Push a new subgraph to keep track of the else branch + current.pushSubgraph(); + current.putSubgraphPred(conditionalDn); + Set elsePreds = visitWithPreds(node.getElseBlock()); + elsePreds.add(conditionalDn); + var elseScope = popConditionalScope(); + current.popSubgraph(); + var succedingScope = mergeConditionalScopes(ifScope, elseScope); + pushConditionalScope(succedingScope); + } else { + // If there is only an if branch then the active symbols after the + // statement + // is the union of the active symbols from before the if and the + // active symbols + // in the if + var succedingScope = mergeConditionalScopes(ifScope, precedingScope); + pushConditionalScope(succedingScope); } } - private Set getAssignmentTargets(Expression node) { - // e.g. (x, y, z) = [1, 2, 3] - if( node instanceof TupleExpression te ) { - return te.getExpressions().stream() - .map(el -> getAssignmentTarget(el).getName()) - .collect(Collectors.toSet()); - } - else { - return Set.of(getAssignmentTarget(node).getName()); + private void visitAssignment(BinaryExpression node) { + var targetExpr = node.getLeftExpression(); + var sourceExpr = node.getRightExpression(); + if( targetExpr instanceof TupleExpression tupleExpr && sourceExpr instanceof ListExpression listExpr ) { + // e.g. (x, y, z) = [1, 2, 3]. + // We need to keep track how what variable is assigned where + List targetExprList = tupleExpr.getExpressions(); + List sourceExprList = listExpr.getExpressions(); + List>> namesAndPreds = new ArrayList<>(); + // We need to wait with adding the nodes, so that we do not overwrite live + // variable names in the source expression + for( int i = 0; i < Math.min(targetExprList.size(), sourceExprList.size()); i++ ) { + String name = getAssignmentTarget(targetExprList.get(i)).getName(); + var preds = visitWithPreds(sourceExprList.get(i)); + namesAndPreds.add(new Tuple2<>(name, preds)); + } + for( var namePreds : namesAndPreds ) { + var name = namePreds.getV1(); + var preds = namePreds.getV2(); + var dn = addNode(name, Node.Type.NAME, null, preds); + putSymbolConditionalScope(name, dn); + } + } else { + processAssignment(targetExpr, sourceExpr); } } + private void processAssignment(Expression target, Expression source) { + String name = getAssignmentTarget(target).getName(); + var preds = visitWithPreds(source); + var dn = addNode(name, Node.Type.NAME, null, preds); + putSymbolConditionalScope(name, dn); + } + private VariableExpression getAssignmentTarget(Expression node) { // e.g. v = 123 if( node instanceof VariableExpression ve ) @@ -238,7 +417,8 @@ private void visitPipeline(BinaryExpression node) { if( defNode instanceof WorkflowNode || defNode instanceof ProcessNode ) { var label = defNode.getName(); var preds = visitWithPreds(lhs); - current.putSymbol(label, addNode(label, Node.Type.OPERATOR, defNode, preds)); + var dn = addNode(label, Node.Type.OPERATOR, defNode, preds); + putSymbolConditionalScope(label, dn); return; } } @@ -288,20 +468,21 @@ private boolean isEntryParam(PropertyExpression node) { return inEntry && node.getObjectExpression() instanceof VariableExpression ve && "params".equals(ve.getName()); } - private Tuple3 asMethodOutput(PropertyExpression node) { + private Tuple3 asMethodOutput(PropertyExpression node) { // named output e.g. PROC.out.foo if( node.getObjectExpression() instanceof PropertyExpression pe ) { if( pe.getObjectExpression() instanceof VariableExpression ve && "out".equals(pe.getPropertyAsString()) ) { var mn = asMethodVariable(ve.getAccessedVariable()); if( mn != null ) - return new Tuple3(mn, ve.getName(), node.getPropertyAsString()); + return new Tuple3(mn, ve.getName(), node.getPropertyAsString()); } } // single output e.g. PROC.out - else if( node.getObjectExpression() instanceof VariableExpression ve && "out".equals(node.getPropertyAsString()) ) { + else if( node.getObjectExpression() instanceof VariableExpression ve + && "out".equals(node.getPropertyAsString()) ) { var mn = asMethodVariable(ve.getAccessedVariable()); if( mn != null ) - return new Tuple3(mn, ve.getName(), null); + return new Tuple3(mn, ve.getName(), null); } return null; } @@ -311,29 +492,21 @@ private void visitProcessOut(ProcessNode process, String label, String propName) addOperatorPred(label, process); return; } - asDirectives(process.outputs) - .filter((call) -> { - var emitName = getProcessEmitName(call); - return propName.equals(emitName); - }) - .findFirst() - .ifPresent((call) -> { - addOperatorPred(label, process); - }); + asDirectives(process.outputs).filter((call) -> { + var emitName = getProcessEmitName(call); + return propName.equals(emitName); + }).findFirst().ifPresent((call) -> { + addOperatorPred(label, process); + }); } private String getProcessEmitName(MethodCallExpression output) { - return Optional.of(output) - .flatMap(call -> Optional.ofNullable(asNamedArgs(call))) - .flatMap(namedArgs -> - namedArgs.stream() - .filter(entry -> "emit".equals(entry.getKeyExpression().getText())) - .findFirst() - ) - .flatMap(entry -> Optional.ofNullable( - entry.getValueExpression() instanceof VariableExpression ve ? ve.getName() : null - )) - .orElse(null); + return Optional.of(output).flatMap(call -> Optional.ofNullable(asNamedArgs(call))) + .flatMap(namedArgs -> namedArgs.stream() + .filter(entry -> "emit".equals(entry.getKeyExpression().getText())).findFirst()) + .flatMap(entry -> Optional + .ofNullable(entry.getValueExpression() instanceof VariableExpression ve ? ve.getName() : null)) + .orElse(null); } private void visitWorkflowOut(WorkflowNode workflow, String label, String propName) { @@ -341,23 +514,19 @@ private void visitWorkflowOut(WorkflowNode workflow, String label, String propNa addOperatorPred(label, workflow); return; } - asBlockStatements(workflow.emits).stream() - .map(stmt -> ((ExpressionStatement) stmt).getExpression()) - .filter((emit) -> { - var emitName = getWorkflowEmitName(emit); - return propName.equals(emitName); - }) - .findFirst() - .ifPresent((call) -> { - addOperatorPred(label, workflow); - }); + asBlockStatements(workflow.emits).stream().map(stmt -> ((ExpressionStatement) stmt).getExpression()) + .filter((emit) -> { + var emitName = getWorkflowEmitName(emit); + return propName.equals(emitName); + }).findFirst().ifPresent((call) -> { + addOperatorPred(label, workflow); + }); } private String getWorkflowEmitName(Expression emit) { if( emit instanceof VariableExpression ve ) { return ve.getName(); - } - else if( emit instanceof AssignmentExpression assign ) { + } else if( emit instanceof AssignmentExpression assign ) { var target = (VariableExpression) assign.getLeftExpression(); return target.getName(); } @@ -365,25 +534,31 @@ else if( emit instanceof AssignmentExpression assign ) { } private void addOperatorPred(String label, ASTNode an) { - var dn = current.getSymbol(label); - if( dn != null ) - currentPreds().add(dn); - else - current.putSymbol(label, addNode(label, Node.Type.OPERATOR, an)); + Set dns = getSymbolConditionalScope(label); + if( dns != null ) { + for( var dn : dns ) { + currentPreds().add(dn); + } + } else { + var dn = addNode(label, Node.Type.OPERATOR, an); + putSymbolConditionalScope(label, dn); + } } @Override public void visitVariableExpression(VariableExpression node) { var name = node.getName(); - var dn = current.getSymbol(name); - if( dn != null ) - currentPreds().add(dn); + Set dns = getSymbolConditionalScope(name); + if( dns != null ) + for( Node dn : dns ) { + currentPreds().add(dn); + } } // helpers private Set currentPreds() { - return stackPreds.lastElement(); + return stackPreds.peek(); } private Set visitWithPreds(ASTNode... nodes) { @@ -392,7 +567,7 @@ private Set visitWithPreds(ASTNode... nodes) { private Set visitWithPreds(Collection nodes) { // traverse a set of nodes and extract predecessor nodes - stackPreds.add(new HashSet<>()); + stackPreds.push(new HashSet<>()); for( var node : nodes ) { if( node != null ) @@ -402,9 +577,9 @@ private Set visitWithPreds(Collection nodes) { return stackPreds.pop(); } - private Node visitWithPred(ASTNode node) { - return visitWithPreds(node).stream().findFirst().orElse(null); - } + // private Node visitWithPred(ASTNode node) { + // return visitWithPreds(node).stream().findFirst().orElse(null); + // } private Node addNode(String label, Node.Type type, ASTNode an, Set preds) { var uri = ast.getURI(an); @@ -414,24 +589,72 @@ private Node addNode(String label, Node.Type type, ASTNode an, Set preds) } private Node addNode(String label, Node.Type type, ASTNode an) { - return addNode(label, type, an, new HashSet<>()); + var dn = addNode(label, type, an, new HashSet<>()); + return dn; } } +class Subgraph { + + private int id; + + private List children = new ArrayList<>(); + + private Set members = new HashSet<>(); + + private Set preds = new HashSet<>(); + + public Subgraph(int id) { + this.id = id; + } + + public int getId() { + return id; + } + + public void addMember(Node n) { + members.add(n); + } + + public Set getMembers() { + return members; + } + + public void addChild(Subgraph s) { + children.add(s); + } + + public List getChildren() { + return children; + } + + public Set getPreds() { + return preds; + } + + public void addPred(Node n) { + preds.add(n); + } +} class Graph { - public final Map inputs = new HashMap<>(); + public final Map inputs = new HashMap<>(); + + public final Map nodes = new HashMap<>(); + + public final Map outputs = new HashMap<>(); - public final Map nodes = new HashMap<>(); + private Stack> scopes = new Stack<>(); - public final Map outputs = new HashMap<>(); + public Stack activeSubgraphs = new Stack<>(); - private List> scopes = new ArrayList<>(); + private int currSubgraphId = 0; public Graph() { pushScope(); + pushSubgraph(); } public void pushScope() { @@ -442,40 +665,44 @@ public void popScope() { scopes.remove(0); } - public Node getSymbol(String name) { - // get a variable node from the name table - for( var scope : scopes ) - if( scope.containsKey(name) ) - return scope.get(name); + public void pushSubgraph() { + activeSubgraphs.push(new Subgraph(currSubgraphId)); + currSubgraphId += 1; + } - return null; + public void popSubgraph() { + var prevSubgraph = activeSubgraphs.pop(); + var currSubgraph = activeSubgraphs.peek(); + currSubgraph.addChild(prevSubgraph); } - public void putSymbol(String name, Node dn) { - // put a variable node into the name table - for( var scope : scopes ) { - if( scope.containsKey(name) ) { - scope.put(name, dn); - return; - } - } + public void putToSubgraph(Node n) { + activeSubgraphs.peek().addMember(n); + } - scopes.get(0).put(name, dn); + public void putSubgraphPred(Node n) { + activeSubgraphs.peek().addPred(n); } - public Node addNode(String label, Node.Type type, URI uri, Set preds) { + public Node addNode(String label, Node.Type type, URI uri, Set preds) { + // Check if the node already exists + // var prevSymbol = getSymbol(label); + // if( prevSymbol != null ) { + // // If it exists, add the predecessors to the existing node + // prevSymbol.addPredecessors(preds); + // return prevSymbol; + // } var id = nodes.size(); var dn = new Node(id, label, type, uri, preds); nodes.put(id, dn); + putToSubgraph(dn); return dn; } } - class Node { public enum Type { - NAME, - OPERATOR + NAME, OPERATOR, CONDITIONAL, IF, ELSE } public final int id; diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index b419cd6..46784f7 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -15,60 +15,76 @@ */ package nextflow.lsp.services.script.dag; +import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage; + import java.util.ArrayList; +import java.util.List; +import java.util.Set; import java.util.Collection; +import java.util.HashSet; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.eclipse.lsp4j.jsonrpc.messages.Tuple; + +import groovy.lang.Tuple2; + /** * * @author Ben Sherman */ public class MermaidRenderer { + private static boolean hideVariables = true; + private static boolean uniqueNames = false; + private Collection inputs = null; + private Collection outputs = null; + public String render(String name, Graph graph) { var isEntry = name == null; var lines = new ArrayList(); lines.add("flowchart TB"); - lines.add(String.format(" subgraph %s", isEntry ? "\" \"" : name)); + lines.add(String.format("subgraph %s", isEntry ? "\" \"" : name)); // prepare inputs and outputs - var inputs = graph.inputs.values(); + inputs = graph.inputs.values(); var nodes = graph.nodes.values(); - var outputs = graph.outputs.values(); + outputs = graph.outputs.values(); // render inputs if( inputs.size() > 0 ) { - lines.add(String.format(" subgraph %s", isEntry ? "params" : "take")); + lines.add(String.format(" subgraph %s", isEntry ? "params" : "take")); for( var dn : inputs ) { if( dn == null ) continue; lines.add(" " + renderNode(dn.id, dn.label, dn.type)); } - lines.add(" end"); + lines.add(" end"); } // render nodes - for( var dn : nodes ) { - if( dn.type == Node.Type.NAME ) - continue; + // for (var dn : nodes) { + // if (isHidden(dn, inputs, outputs)) + // continue; - var label = dn.label - .replaceAll("\n", "\\\\\n") - .replaceAll("\"", "\\\\\""); + // var label = dn.label + // .replaceAll("\n", "\\\\\n") + // .replaceAll("\"", "\\\\\""); - lines.add(" " + renderNode(dn.id, label, dn.type)); + // lines.add(" " + renderNode(dn.id, label, dn.type)); - if( dn.uri != null ) - lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, dn.uri.toString())); - } + // if (dn.uri != null) + // lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, + // dn.uri.toString())); + // } + var subgraphEdges = renderSubgraphs(graph.activeSubgraphs.peek(), lines, 0); // render outputs if( outputs.size() > 0 ) { - lines.add(String.format(" subgraph %s", isEntry ? "publish" : "emit")); + lines.add(String.format(" subgraph %s", isEntry ? "publish" : "emit")); for( var dn : outputs ) lines.add(" " + renderNode(dn.id, dn.label, dn.type)); - lines.add(" end"); + lines.add(" end"); } // render edges @@ -81,24 +97,56 @@ public String render(String name, Graph graph) { var done = preds.stream().allMatch(p -> !isHidden(p, inputs, outputs)); if( done ) break; - preds = preds.stream() - .flatMap(p -> - isHidden(p, inputs, outputs) - ? p.preds.stream() - : Stream.of(p) - ) - .collect(Collectors.toSet()); + preds = preds.stream().flatMap(p -> isHidden(p, inputs, outputs) ? p.preds.stream() : Stream.of(p)) + .collect(Collectors.toSet()); } for( var dnPred : preds ) lines.add(String.format(" v%d --> v%d", dnPred.id, dn.id)); } - lines.add(" end"); + for( var e : subgraphEdges ) { + lines.add(String.format(" v%d --> s%d", e.getV2(), e.getV1())); + } + + lines.add("end"); return String.join("\n", lines); } + // Write the subgraphs and fetch the list of subgraph edges + private Set> renderSubgraphs(Subgraph s, ArrayList lines, int depth) { + + if( depth > 0 ) { + lines.add(" ".repeat(depth) + String.format("subgraph s%d[\" \"]", s.getId())); + } + for( var dn : s.getMembers() ) { + if( isHidden(dn, inputs, outputs) ) { + continue; + } + + var label = dn.label.replaceAll("\n", "\\\\\n").replaceAll("\"", "\\\\\""); + + lines.add(" ".repeat(depth + 1) + renderNode(dn.id, label, dn.type)); + if( dn.uri != null ) { + lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, dn.uri.toString())); + } + } + // Get incoming edges + Set> incidentEdges = new HashSet<>(); + for( var p : s.getPreds() ) { + incidentEdges.add(new Tuple2(s.getId(), p.id)); + } + for( var child : s.getChildren() ) { + var moreEdges = renderSubgraphs(child, lines, depth + 1); + incidentEdges.addAll(moreEdges); + } + if( depth > 0 ) { + lines.add(" ".repeat(depth) + "end"); + } + return incidentEdges; + } + /** * Only inputs, outputs, and processes/workflows are currently shown. * @@ -107,13 +155,22 @@ public String render(String name, Graph graph) { * @param outputs */ private static boolean isHidden(Node dn, Collection inputs, Collection outputs) { - return dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn); + return hideVariables && dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn); } private static String renderNode(int id, String label, Node.Type type) { - return switch( type ) { - case NAME -> String.format("v%d[\"%s\"]", id, label); - case OPERATOR -> String.format("v%d([%s])", id, label); + String name; + if( uniqueNames ) { + name = String.format("%s<%d>", label, id); + } else { + name = String.format("%s", label); + } + return switch (type) { + case NAME -> String.format("v%d[\"%s\"]", id, name); + case OPERATOR -> String.format("v%d([%s])", id, name); + case CONDITIONAL -> String.format("v%d([conditional])", id); + case IF -> String.format("v%d([if])", id); + case ELSE -> String.format("v%d([else])", id); }; } From 14b67f38029c7219315a2ef5dc155e2310df8303 Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Tue, 15 Jul 2025 11:33:09 +0200 Subject: [PATCH 03/15] Fix local variables --- .../services/script/dag/DataflowVisitor.java | 193 ++++++++++++------ .../services/script/dag/MermaidRenderer.java | 22 +- 2 files changed, 134 insertions(+), 81 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index 59d6bbb..f9c7233 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -52,26 +52,36 @@ */ class Variable { - private int defintionDepth; + private int definitionDepth; private Set activeInstances; + private Node.Type type; - public Variable(Node activeInstance, int defintionDepth) { + public Variable(Node activeInstance, int definitionDepth) { this.activeInstances = new HashSet<>(); activeInstances.add(activeInstance); - this.defintionDepth = defintionDepth; + this.definitionDepth = definitionDepth; + type = activeInstance.type; } - private Variable(Set activeInstances, int defintionDepth) { + private Variable(Set activeInstances, int definitionDepth) { this.activeInstances = new HashSet<>(activeInstances); - this.defintionDepth = defintionDepth; + this.definitionDepth = definitionDepth; + } + + public Node.Type getType() { + return type; } public Variable shallowCopy() { - return new Variable(activeInstances, defintionDepth); + return new Variable(activeInstances, definitionDepth); } public boolean isLocal(int currDepth) { - return defintionDepth == currDepth; + return definitionDepth == currDepth; + } + + public int getDefinitionDepth() { + return definitionDepth; } public Set getActiveInstances() { @@ -86,7 +96,7 @@ public void setActiveInstance(Node dn) { public Variable union(Variable other) { var unionInstances = new HashSet<>(activeInstances); unionInstances.addAll(other.getActiveInstances()); - return new Variable(unionInstances, defintionDepth); + return new Variable(unionInstances, definitionDepth); } } @@ -147,68 +157,95 @@ public Set getSymbolConditionalScope(String name) { } } - public void putSymbolConditionalScope(String name, Node dn) { + public void putSymbolConditionalScope(String name, Node dn, boolean isLocal) { + // Put a symbol into the currently active symbol table var scope = conditionalScopes.peek(); + if( scope.containsKey(name) ) { + // We have reassinged the variable, which means that all previous // definitions of it (in this scope) are dead. We can reuse the set // though Variable variable = scope.get(name); variable.setActiveInstance(dn); + } else { + // The symbols in not present from before so create an empty set - Variable variable = new Variable(dn, currentDepth); + Variable variable = new Variable(dn, isLocal ? currentDepth : 0); scope.put(name, variable); + } } public void pushConditionalScope(Map previousSymbols) { + // We want to add all symbols in the current scope // since they always enter both branches var newScope = new HashMap(); + for( Map.Entry entry : previousSymbols.entrySet() ) { newScope.put(entry.getKey(), entry.getValue().shallowCopy()); } + conditionalScopes.push(newScope); - currentDepth += 1; + } + + public Map popConditionalScope() { + return conditionalScopes.pop(); } public Map mergeConditionalScopes(Map ifSymbols, - Map elseSymbols) { - // Merge the set nodes corresponding to active symbols for the two - // scopes - // NOTE: Since we can define local variables with `def` we should have a - // flag - // which tells us - // if this symbol is locally defined in this scope or defined outside - // the scope. + Map elseSymbols, Subgraph ifSubgraph, Subgraph elseSubgraph) { + // Merge the set nodes corresponding to active symbols for the two scopes var merged = new HashMap(); // Add all keys from ifSymbols for( Map.Entry entry : ifSymbols.entrySet() ) { String key = entry.getKey(); Variable ifVariable = entry.getValue(); - if( elseSymbols.containsKey(key) ) { - merged.put(key, ifVariable.union(elseSymbols.get(key))); - } else { - merged.put(key, ifVariable.shallowCopy()); + if( ifVariable.getDefinitionDepth() <= currentDepth ) { + if( elseSymbols.containsKey(key) ) { + merged.put(key, ifVariable.union(elseSymbols.get(key))); + } else { + // The variable is only found in the else branch, so we should add a null + // initializer to it + if( ifVariable.getType() == Node.Type.NAME ) { + Node uninitNode = fixUninitNodeSubgraph(key, elseSubgraph); + merged.put(key, ifVariable.union(new Variable(uninitNode, 0))); + } else { + merged.put(key, ifVariable.shallowCopy()); + } + } } } // Add remaining keys from elseSymbols (already handled common ones) for( Map.Entry entry : elseSymbols.entrySet() ) { String key = entry.getKey(); - if( !merged.containsKey(key) ) { - merged.put(key, entry.getValue().shallowCopy()); + Variable elseVariable = entry.getValue(); + if( !merged.containsKey(key) && elseVariable.getDefinitionDepth() <= currentDepth ) { + // The variable is only found in the if branch, so we should add a null + // initializer to it + if( elseVariable.getType() == Node.Type.NAME ) { + Node uninitNode = fixUninitNodeSubgraph(key, ifSubgraph); + merged.put(key, elseVariable.union(new Variable(uninitNode, 0))); + } else { + merged.put(key, elseVariable.shallowCopy()); + } } } return merged; } - public Map popConditionalScope() { - currentDepth -= 1; - return conditionalScopes.pop(); + private Node fixUninitNodeSubgraph(String label, Subgraph subgraph) { + var nullAndUninitNode = addUninitalizedNode(label, null); + var nullNode = nullAndUninitNode.getV1(); + var uninitNode = nullAndUninitNode.getV2(); + subgraph.addMember(nullNode); + subgraph.addMember(uninitNode); + return uninitNode; } @Override @@ -231,7 +268,7 @@ private void visitWorkflowTakes(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.takes) ) { var name = asVarX(stmt).getName(); var dn = addNode(name, Node.Type.NAME, stmt); - putSymbolConditionalScope(name, dn); + putSymbolConditionalScope(name, dn, false); result.put(name, dn); } } @@ -297,7 +334,7 @@ public void visitMethodCallExpression(MethodCallExpression node) { if( defNode instanceof WorkflowNode || defNode instanceof ProcessNode ) { var preds = visitWithPreds(node.getArguments()); var dn = addNode(name, Node.Type.OPERATOR, defNode, preds); - putSymbolConditionalScope(name, dn); + putSymbolConditionalScope(name, dn, false); return; } @@ -307,7 +344,7 @@ public void visitMethodCallExpression(MethodCallExpression node) { @Override public void visitBinaryExpression(BinaryExpression node) { if( node instanceof AssignmentExpression ) { - visitAssignment(node); + visitAssignment(node, false); return; } if( node.getOperation().getType() == Types.PIPE ) { @@ -320,6 +357,7 @@ public void visitBinaryExpression(BinaryExpression node) { @Override public void visitIfElse(IfStatement node) { + // First visit the conditional var preds = visitWithPreds(node.getBooleanExpression()); var conditionalDn = addNode("", Node.Type.CONDITIONAL, node, preds); @@ -328,70 +366,92 @@ public void visitIfElse(IfStatement node) { // Get the symbol table associated with the currently available global // variables var precedingScope = popConditionalScope(); + // Construct the scope for the if branch pushConditionalScope(precedingScope); - // Push a new subgraph to keep track of the if branch + currentDepth += 1; + current.pushSubgraph(); current.putSubgraphPred(conditionalDn); + visitWithPreds(node.getIfBlock()); + var ifScope = popConditionalScope(); - current.popSubgraph(); + var ifSubgraph = current.popSubgraph(); + + Map elseScope; + Subgraph elseSubgraph; if( !node.getElseBlock().isEmpty() ) { + // We push a new symbol table to keep track of the // symbols in the else branch. pushConditionalScope(precedingScope); + // Push a new subgraph to keep track of the else branch current.pushSubgraph(); current.putSubgraphPred(conditionalDn); - Set elsePreds = visitWithPreds(node.getElseBlock()); - elsePreds.add(conditionalDn); - var elseScope = popConditionalScope(); - current.popSubgraph(); - var succedingScope = mergeConditionalScopes(ifScope, elseScope); - pushConditionalScope(succedingScope); + visitWithPreds(node.getElseBlock()); + + // Exit the else branch + elseScope = popConditionalScope(); + elseSubgraph = current.popSubgraph(); + } else { - // If there is only an if branch then the active symbols after the - // statement - // is the union of the active symbols from before the if and the - // active symbols + + // If there is only an if branch then the active symbols after the statement + // is the union of the active symbols from before the if and the active symbols // in the if - var succedingScope = mergeConditionalScopes(ifScope, precedingScope); - pushConditionalScope(succedingScope); + elseScope = precedingScope; + elseSubgraph = current.peekSubgraph(); + } + + currentDepth -= 1; + var succedingScope = mergeConditionalScopes(ifScope, elseScope, ifSubgraph, elseSubgraph); + pushConditionalScope(succedingScope); + } - private void visitAssignment(BinaryExpression node) { + private void visitAssignment(BinaryExpression node, boolean isDef) { + var targetExpr = node.getLeftExpression(); var sourceExpr = node.getRightExpression(); + if( targetExpr instanceof TupleExpression tupleExpr && sourceExpr instanceof ListExpression listExpr ) { + // e.g. (x, y, z) = [1, 2, 3]. - // We need to keep track how what variable is assigned where + // We need to keep track what variable is assigned where List targetExprList = tupleExpr.getExpressions(); List sourceExprList = listExpr.getExpressions(); - List>> namesAndPreds = new ArrayList<>(); + // We need to wait with adding the nodes, so that we do not overwrite live // variable names in the source expression + List>> namesAndPreds = new ArrayList<>(); + for( int i = 0; i < Math.min(targetExprList.size(), sourceExprList.size()); i++ ) { String name = getAssignmentTarget(targetExprList.get(i)).getName(); var preds = visitWithPreds(sourceExprList.get(i)); namesAndPreds.add(new Tuple2<>(name, preds)); } + for( var namePreds : namesAndPreds ) { var name = namePreds.getV1(); var preds = namePreds.getV2(); var dn = addNode(name, Node.Type.NAME, null, preds); - putSymbolConditionalScope(name, dn); + putSymbolConditionalScope(name, dn, isDef); } } else { - processAssignment(targetExpr, sourceExpr); + + processAssignment(targetExpr, sourceExpr, isDef); + } } - private void processAssignment(Expression target, Expression source) { + private void processAssignment(Expression target, Expression source, boolean isDef) { String name = getAssignmentTarget(target).getName(); var preds = visitWithPreds(source); var dn = addNode(name, Node.Type.NAME, null, preds); - putSymbolConditionalScope(name, dn); + putSymbolConditionalScope(name, dn, isDef); } private VariableExpression getAssignmentTarget(Expression node) { @@ -418,7 +478,7 @@ private void visitPipeline(BinaryExpression node) { var label = defNode.getName(); var preds = visitWithPreds(lhs); var dn = addNode(label, Node.Type.OPERATOR, defNode, preds); - putSymbolConditionalScope(label, dn); + putSymbolConditionalScope(label, dn, false); return; } } @@ -428,7 +488,7 @@ private void visitPipeline(BinaryExpression node) { @Override public void visitDeclarationExpression(DeclarationExpression node) { - visitAssignment(node); + visitAssignment(node, true); } @Override @@ -541,7 +601,7 @@ private void addOperatorPred(String label, ASTNode an) { } } else { var dn = addNode(label, Node.Type.OPERATOR, an); - putSymbolConditionalScope(label, dn); + putSymbolConditionalScope(label, dn, false); } } @@ -593,6 +653,12 @@ private Node addNode(String label, Node.Type type, ASTNode an) { return dn; } + private Tuple2 addUninitalizedNode(String label, ASTNode an) { + var nullNode = addNode("null", Node.Type.NULL, an); + var uninitalizedNode = current.addNode(label, Node.Type.NAME, null, Set.of(nullNode)); + return new Tuple2<>(nullNode, uninitalizedNode); + } + } class Subgraph { @@ -670,10 +736,15 @@ public void pushSubgraph() { currSubgraphId += 1; } - public void popSubgraph() { + public Subgraph popSubgraph() { var prevSubgraph = activeSubgraphs.pop(); var currSubgraph = activeSubgraphs.peek(); currSubgraph.addChild(prevSubgraph); + return prevSubgraph; + } + + public Subgraph peekSubgraph() { + return activeSubgraphs.peek(); } public void putToSubgraph(Node n) { @@ -685,24 +756,18 @@ public void putSubgraphPred(Node n) { } public Node addNode(String label, Node.Type type, URI uri, Set preds) { - // Check if the node already exists - // var prevSymbol = getSymbol(label); - // if( prevSymbol != null ) { - // // If it exists, add the predecessors to the existing node - // prevSymbol.addPredecessors(preds); - // return prevSymbol; - // } var id = nodes.size(); var dn = new Node(id, label, type, uri, preds); nodes.put(id, dn); putToSubgraph(dn); return dn; } + } class Node { public enum Type { - NAME, OPERATOR, CONDITIONAL, IF, ELSE + NAME, OPERATOR, CONDITIONAL, NULL } public final int id; diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 46784f7..9dee966 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -40,6 +40,10 @@ public class MermaidRenderer { private Collection inputs = null; private Collection outputs = null; + public void toggleVariableHiding() { + hideVariables = !hideVariables; + } + public String render(String name, Graph graph) { var isEntry = name == null; var lines = new ArrayList(); @@ -62,21 +66,6 @@ public String render(String name, Graph graph) { lines.add(" end"); } - // render nodes - // for (var dn : nodes) { - // if (isHidden(dn, inputs, outputs)) - // continue; - - // var label = dn.label - // .replaceAll("\n", "\\\\\n") - // .replaceAll("\"", "\\\\\""); - - // lines.add(" " + renderNode(dn.id, label, dn.type)); - - // if (dn.uri != null) - // lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, - // dn.uri.toString())); - // } var subgraphEdges = renderSubgraphs(graph.activeSubgraphs.peek(), lines, 0); // render outputs @@ -169,8 +158,7 @@ private static String renderNode(int id, String label, Node.Type type) { case NAME -> String.format("v%d[\"%s\"]", id, name); case OPERATOR -> String.format("v%d([%s])", id, name); case CONDITIONAL -> String.format("v%d([conditional])", id); - case IF -> String.format("v%d([if])", id); - case ELSE -> String.format("v%d([else])", id); + case NULL -> String.format("v%d([null])", id); }; } From 287324c9ad54bd41ff89f6fc947cdb503456cbc3 Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Tue, 15 Jul 2025 12:33:56 +0200 Subject: [PATCH 04/15] Add configuration for showing variables or not --- .../java/nextflow/lsp/NextflowLanguageServer.java | 8 +++++--- .../lsp/services/LanguageServerConfiguration.java | 4 +++- .../java/nextflow/lsp/services/LanguageService.java | 6 +++--- .../lsp/services/script/ScriptCodeLensProvider.java | 6 ++++-- .../nextflow/lsp/services/script/ScriptService.java | 9 +++++---- .../lsp/services/script/dag/MermaidRenderer.java | 13 +++++++------ 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/main/java/nextflow/lsp/NextflowLanguageServer.java b/src/main/java/nextflow/lsp/NextflowLanguageServer.java index 84562c1..91c3bcd 100644 --- a/src/main/java/nextflow/lsp/NextflowLanguageServer.java +++ b/src/main/java/nextflow/lsp/NextflowLanguageServer.java @@ -306,7 +306,7 @@ public CompletableFuture> codeLens(CodeLensParams param var service = getLanguageService(uri); if( service == null ) return Collections.emptyList(); - return service.codeLens(params); + return service.codeLens(params, configuration.showVariablesInDAG()); }); } @@ -457,7 +457,8 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) { withDefault(JsonUtils.getBoolean(settings, "nextflow.formatting.maheshForm"), configuration.maheshForm()), withDefault(JsonUtils.getInteger(settings, "nextflow.completion.maxItems"), configuration.maxCompletionItems()), withDefault(JsonUtils.getBoolean(settings, "nextflow.formatting.sortDeclarations"), configuration.sortDeclarations()), - withDefault(JsonUtils.getBoolean(settings, "nextflow.typeChecking"), configuration.typeChecking()) + withDefault(JsonUtils.getBoolean(settings, "nextflow.typeChecking"), configuration.typeChecking()), + withDefault(JsonUtils.getBoolean(settings, "nextflow.dag.showVariables"), configuration.showVariablesInDAG()) ); if( shouldInitialize(oldConfiguration, configuration) ) @@ -547,8 +548,9 @@ public CompletableFuture executeCommand(ExecuteCommandParams params) { return CompletableFutures.computeAsync((cancelChecker) -> { cancelChecker.checkCanceled(); var command = params.getCommand(); - var arguments = params.getArguments(); + List arguments = params.getArguments(); if( "nextflow.server.previewDag".equals(command) && arguments.size() == 2 ) { + arguments.add(configuration.showVariablesInDAG()); log.debug(String.format("textDocument/previewDag %s", arguments.toString())); var uri = JsonUtils.getString(arguments.get(0)); var service = getLanguageService(uri); diff --git a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java index 2c603ed..d458f26 100644 --- a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java +++ b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java @@ -26,7 +26,8 @@ public record LanguageServerConfiguration( boolean maheshForm, int maxCompletionItems, boolean sortDeclarations, - boolean typeChecking + boolean typeChecking, + boolean showVariablesInDAG ) { public static LanguageServerConfiguration defaults() { @@ -38,6 +39,7 @@ public static LanguageServerConfiguration defaults() { false, 100, false, + false, false ); } diff --git a/src/main/java/nextflow/lsp/services/LanguageService.java b/src/main/java/nextflow/lsp/services/LanguageService.java index bf352e0..4edbb76 100644 --- a/src/main/java/nextflow/lsp/services/LanguageService.java +++ b/src/main/java/nextflow/lsp/services/LanguageService.java @@ -109,7 +109,7 @@ public LanguageService(String rootUri) { public abstract boolean matchesFile(String uri); protected abstract ASTNodeCache getAstCache(); protected CallHierarchyProvider getCallHierarchyProvider() { return null; } - protected CodeLensProvider getCodeLensProvider() { return null; } + protected CodeLensProvider getCodeLensProvider(boolean showVariablesInDAG) { return null; } protected CompletionProvider getCompletionProvider(int maxItems, boolean extended) { return null; } protected DefinitionProvider getDefinitionProvider() { return null; } protected FormattingProvider getFormattingProvider() { return null; } @@ -186,8 +186,8 @@ public List callHierarchyOutgoingCalls(CallHierarchyI return provider.outgoingCalls(item); } - public List codeLens(CodeLensParams params) { - var provider = getCodeLensProvider(); + public List codeLens(CodeLensParams params, boolean showVariablesInDAG) { + var provider = getCodeLensProvider(showVariablesInDAG); if( provider == null ) return Collections.emptyList(); diff --git a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java index 7073cf4..f163306 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java @@ -41,9 +41,11 @@ public class ScriptCodeLensProvider implements CodeLensProvider { private static Logger log = Logger.getInstance(); private ScriptAstCache ast; + private boolean showVariablesInDAG; - public ScriptCodeLensProvider(ScriptAstCache ast) { + public ScriptCodeLensProvider(ScriptAstCache ast, boolean showVariablesInDAG) { this.ast = ast; + this.showVariablesInDAG = showVariablesInDAG; } @Override @@ -84,7 +86,7 @@ public Map previewDag(String documentUri, String name) { visitor.visit(); var graph = visitor.getGraph(wn.isEntry() ? "" : wn.getName()); - var result = new MermaidRenderer().render(wn.getName(), graph); + var result = new MermaidRenderer(!showVariablesInDAG).render(wn.getName(), graph); log.debug(result); return Map.ofEntries(Map.entry("result", result)); }) diff --git a/src/main/java/nextflow/lsp/services/script/ScriptService.java b/src/main/java/nextflow/lsp/services/script/ScriptService.java index a4ead65..be26daf 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptService.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptService.java @@ -71,8 +71,8 @@ protected CallHierarchyProvider getCallHierarchyProvider() { } @Override - protected CodeLensProvider getCodeLensProvider() { - return new ScriptCodeLensProvider(astCache); + protected CodeLensProvider getCodeLensProvider(boolean showVariablesInDAG) { + return new ScriptCodeLensProvider(astCache, showVariablesInDAG); } @Override @@ -123,10 +123,11 @@ protected SymbolProvider getSymbolProvider() { @Override public Object executeCommand(String command, List arguments) { updateNow(); - if( "nextflow.server.previewDag".equals(command) && arguments.size() == 2 ) { + if( "nextflow.server.previewDag".equals(command) && arguments.size() == 3 ) { var uri = getJsonString(arguments.get(0)); var name = getJsonString(arguments.get(1)); - var provider = new ScriptCodeLensProvider(astCache); + boolean showVariablesInDAG = (boolean)arguments.get(2); + var provider = new ScriptCodeLensProvider(astCache, showVariablesInDAG); return provider.previewDag(uri, name); } if( "nextflow.server.previewWorkspace".equals(command) ) { diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 9dee966..47bd241 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -35,13 +35,14 @@ */ public class MermaidRenderer { - private static boolean hideVariables = true; - private static boolean uniqueNames = false; + private final boolean hideVariables; + private final boolean uniqueNames; private Collection inputs = null; private Collection outputs = null; - public void toggleVariableHiding() { - hideVariables = !hideVariables; + public MermaidRenderer(boolean hideVariables) { + this.hideVariables = hideVariables; + this.uniqueNames = false; } public String render(String name, Graph graph) { @@ -143,11 +144,11 @@ private Set> renderSubgraphs(Subgraph s, ArrayList inputs, Collection outputs) { + private boolean isHidden(Node dn, Collection inputs, Collection outputs) { return hideVariables && dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn); } - private static String renderNode(int id, String label, Node.Type type) { + private String renderNode(int id, String label, Node.Type type) { String name; if( uniqueNames ) { name = String.format("%s<%d>", label, id); From 737082b8b650442cd3b358384a5828cf7ce30f3d Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Tue, 15 Jul 2025 15:43:38 +0200 Subject: [PATCH 05/15] Add better support for collapsing graph --- .../services/script/dag/DataflowVisitor.java | 53 ++++++++++++++++++- .../services/script/dag/MermaidRenderer.java | 51 ++++++++---------- 2 files changed, 75 insertions(+), 29 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index f9c7233..783e60d 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -25,6 +25,7 @@ import java.util.Set; import java.util.Stack; import java.util.stream.Collectors; +import java.util.function.Function; import groovy.lang.Tuple2; import groovy.lang.Tuple3; @@ -655,7 +656,9 @@ private Node addNode(String label, Node.Type type, ASTNode an) { private Tuple2 addUninitalizedNode(String label, ASTNode an) { var nullNode = addNode("null", Node.Type.NULL, an); - var uninitalizedNode = current.addNode(label, Node.Type.NAME, null, Set.of(nullNode)); + var preds = new HashSet(); + preds.add(nullNode); + var uninitalizedNode = current.addNode(label, Node.Type.NAME, null, preds); return new Tuple2<>(nullNode, uninitalizedNode); } @@ -763,6 +766,54 @@ public Node addNode(String label, Node.Type type, URI uri, Set preds) { return dn; } + public void collapseGraph(Function hide, Function hideIfDisconnected) { + // We want to collapse the graph by removing all nodes satisfying the predicate `isHidden` + // and then look for disconnected nodes which should be hidden if they satisfy `hideIfDisconnected` + Set visited = new HashSet<>(); + Set remove = new HashSet<>(); + Set maybeRemove = new HashSet<>(); + for (Map.Entry idNode : nodes.entrySet()) { + int id = idNode.getKey(); + Node node = idNode.getValue(); + if (!hide.apply(node)) { + if (!hideIfDisconnected.apply(node)) { + findTrueAncestors(node, hide, visited); + visited.add(id); + } else { + maybeRemove.add(id); + } + } else { + remove.add(id); + } + } + // If a node was visited then it is necessarily connected to a node we are keeping + maybeRemove.removeAll(visited); + remove.addAll(maybeRemove); + + for (int id : remove) + nodes.remove(id); + + } + + private Set findTrueAncestors(Node node, Function hide, Set visited) { + Set truePreds = new HashSet<>(); + for (Node pred : node.preds) { + if (!hide.apply(pred)) { + truePreds.add(pred); + } else if (!visited.contains(pred.id)) { + var predAncestors = findTrueAncestors(pred, hide, visited); + truePreds.addAll(predAncestors); + } else { + // We have already visited the node and have found its ancestors + truePreds.addAll(pred.preds); + } + visited.add(pred.id); + } + node.preds.clear(); + node.preds.addAll(truePreds); + return truePreds; + } + } class Node { diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 47bd241..073d975 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -24,6 +24,7 @@ import java.util.HashSet; import java.util.stream.Collectors; import java.util.stream.Stream; +import java.util.function.Function; import org.eclipse.lsp4j.jsonrpc.messages.Tuple; @@ -50,12 +51,15 @@ public String render(String name, Graph graph) { var lines = new ArrayList(); lines.add("flowchart TB"); lines.add(String.format("subgraph %s", isEntry ? "\" \"" : name)); - + // prepare inputs and outputs inputs = graph.inputs.values(); var nodes = graph.nodes.values(); outputs = graph.outputs.values(); + // Collapse graph + graph.collapseGraph(isHidden(inputs, outputs), isHiddenIfDisconnected()); + // render inputs if( inputs.size() > 0 ) { lines.add(String.format(" subgraph %s", isEntry ? "params" : "take")); @@ -67,7 +71,7 @@ public String render(String name, Graph graph) { lines.add(" end"); } - var subgraphEdges = renderSubgraphs(graph.activeSubgraphs.peek(), lines, 0); + var subgraphEdges = renderSubgraphs(graph.activeSubgraphs.peek(), graph, lines, 0); // render outputs if( outputs.size() > 0 ) { @@ -78,22 +82,9 @@ public String render(String name, Graph graph) { } // render edges - for( var dn : nodes ) { - if( isHidden(dn, inputs, outputs) ) - continue; - - var preds = dn.preds; - while( true ) { - var done = preds.stream().allMatch(p -> !isHidden(p, inputs, outputs)); - if( done ) - break; - preds = preds.stream().flatMap(p -> isHidden(p, inputs, outputs) ? p.preds.stream() : Stream.of(p)) - .collect(Collectors.toSet()); - } - - for( var dnPred : preds ) + for( var dn : nodes ) + for( var dnPred : dn.preds ) lines.add(String.format(" v%d --> v%d", dnPred.id, dn.id)); - } for( var e : subgraphEdges ) { lines.add(String.format(" v%d --> s%d", e.getV2(), e.getV1())); @@ -105,21 +96,21 @@ public String render(String name, Graph graph) { } // Write the subgraphs and fetch the list of subgraph edges - private Set> renderSubgraphs(Subgraph s, ArrayList lines, int depth) { + private Set> renderSubgraphs(Subgraph s, Graph g, ArrayList lines, int depth) { if( depth > 0 ) { lines.add(" ".repeat(depth) + String.format("subgraph s%d[\" \"]", s.getId())); } for( var dn : s.getMembers() ) { - if( isHidden(dn, inputs, outputs) ) { - continue; - } + if (g.nodes.containsKey(dn.id)) { + + var label = dn.label.replaceAll("\n", "\\\\\n").replaceAll("\"", "\\\\\""); - var label = dn.label.replaceAll("\n", "\\\\\n").replaceAll("\"", "\\\\\""); + lines.add(" ".repeat(depth + 1) + renderNode(dn.id, label, dn.type)); + if( dn.uri != null ) { + lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, dn.uri.toString())); + } - lines.add(" ".repeat(depth + 1) + renderNode(dn.id, label, dn.type)); - if( dn.uri != null ) { - lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, dn.uri.toString())); } } // Get incoming edges @@ -128,7 +119,7 @@ private Set> renderSubgraphs(Subgraph s, ArrayList(s.getId(), p.id)); } for( var child : s.getChildren() ) { - var moreEdges = renderSubgraphs(child, lines, depth + 1); + var moreEdges = renderSubgraphs(child, g, lines, depth + 1); incidentEdges.addAll(moreEdges); } if( depth > 0 ) { @@ -144,8 +135,12 @@ private Set> renderSubgraphs(Subgraph s, ArrayList inputs, Collection outputs) { - return hideVariables && dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn); + private Function isHidden(Collection inputs, Collection outputs) { + return (dn -> hideVariables && dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn)); + } + + private Function isHiddenIfDisconnected() { + return (dn -> hideVariables && dn.type == Node.Type.NULL); } private String renderNode(int id, String label, Node.Type type) { From ffec1f549d5bd6aa1ff27503b8c221f3a63300f5 Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Tue, 15 Jul 2025 15:56:21 +0200 Subject: [PATCH 06/15] Add myself as @author (?) and move some things around --- .../services/script/dag/DataflowVisitor.java | 92 +++++++++---------- .../services/script/dag/MermaidRenderer.java | 10 +- 2 files changed, 46 insertions(+), 56 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index 783e60d..0f4f629 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -24,12 +24,10 @@ import java.util.Optional; import java.util.Set; import java.util.Stack; -import java.util.stream.Collectors; import java.util.function.Function; import groovy.lang.Tuple2; import groovy.lang.Tuple3; -import nextflow.lsp.ast.LanguageServerASTUtils; import nextflow.lsp.services.script.ScriptAstCache; import nextflow.script.ast.ASTNodeMarker; import nextflow.script.ast.AssignmentExpression; @@ -50,6 +48,7 @@ /** * * @author Ben Sherman + * @author Erik Danielsson */ class Variable { @@ -638,10 +637,6 @@ private Set visitWithPreds(Collection nodes) { return stackPreds.pop(); } - // private Node visitWithPred(ASTNode node) { - // return visitWithPreds(node).stream().findFirst().orElse(null); - // } - private Node addNode(String label, Node.Type type, ASTNode an, Set preds) { var uri = ast.getURI(an); var dn = current.addNode(label, type, uri, preds); @@ -664,49 +659,6 @@ private Tuple2 addUninitalizedNode(String label, ASTNode an) { } -class Subgraph { - - private int id; - - private List children = new ArrayList<>(); - - private Set members = new HashSet<>(); - - private Set preds = new HashSet<>(); - - public Subgraph(int id) { - this.id = id; - } - - public int getId() { - return id; - } - - public void addMember(Node n) { - members.add(n); - } - - public Set getMembers() { - return members; - } - - public void addChild(Subgraph s) { - children.add(s); - } - - public List getChildren() { - return children; - } - - public Set getPreds() { - return preds; - } - - public void addPred(Node n) { - preds.add(n); - } -} - class Graph { public final Map inputs = new HashMap<>(); @@ -777,6 +729,7 @@ public void collapseGraph(Function hide, Function Node node = idNode.getValue(); if (!hide.apply(node)) { if (!hideIfDisconnected.apply(node)) { + // We have found a node that should always be connected, start searching from here findTrueAncestors(node, hide, visited); visited.add(id); } else { @@ -815,7 +768,48 @@ private Set findTrueAncestors(Node node, Function hide, Set } } +class Subgraph { + + private int id; + + private List children = new ArrayList<>(); + + private Set members = new HashSet<>(); + + private Set preds = new HashSet<>(); + + public Subgraph(int id) { + this.id = id; + } + + public int getId() { + return id; + } + + public void addMember(Node n) { + members.add(n); + } + + public Set getMembers() { + return members; + } + public void addChild(Subgraph s) { + children.add(s); + } + + public List getChildren() { + return children; + } + + public Set getPreds() { + return preds; + } + + public void addPred(Node n) { + preds.add(n); + } +} class Node { public enum Type { NAME, OPERATOR, CONDITIONAL, NULL diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 073d975..2890b2f 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -15,24 +15,20 @@ */ package nextflow.lsp.services.script.dag; -import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage; import java.util.ArrayList; -import java.util.List; import java.util.Set; import java.util.Collection; import java.util.HashSet; -import java.util.stream.Collectors; -import java.util.stream.Stream; import java.util.function.Function; -import org.eclipse.lsp4j.jsonrpc.messages.Tuple; import groovy.lang.Tuple2; /** * * @author Ben Sherman + * @author Erik Danielsson */ public class MermaidRenderer { @@ -57,7 +53,7 @@ public String render(String name, Graph graph) { var nodes = graph.nodes.values(); outputs = graph.outputs.values(); - // Collapse graph + // Collapse graph i.e. remove nodes that shouldn't be part of the layout graph.collapseGraph(isHidden(inputs, outputs), isHiddenIfDisconnected()); // render inputs @@ -154,7 +150,7 @@ private String renderNode(int id, String label, Node.Type type) { case NAME -> String.format("v%d[\"%s\"]", id, name); case OPERATOR -> String.format("v%d([%s])", id, name); case CONDITIONAL -> String.format("v%d([conditional])", id); - case NULL -> String.format("v%d([null])", id); + case NULL -> String.format("v%d{null}", id); }; } From fe057cd7ca58bf8e7e29cfd750661ee5fc8449af Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Wed, 30 Jul 2025 12:53:53 -0500 Subject: [PATCH 07/15] cleanup --- .../nextflow/lsp/NextflowLanguageServer.java | 8 +- .../services/LanguageServerConfiguration.java | 4 +- .../lsp/services/LanguageService.java | 6 +- .../script/ScriptCodeLensProvider.java | 10 +- .../lsp/services/script/ScriptService.java | 9 +- .../services/script/dag/DataflowVisitor.java | 656 ++++-------------- .../lsp/services/script/dag/Graph.java | 135 ++++ .../services/script/dag/MermaidRenderer.java | 193 ++++-- .../services/script/dag/VariableContext.java | 168 +++++ .../services/script/dag/PreviewDagTest.groovy | 168 +++++ 10 files changed, 740 insertions(+), 617 deletions(-) create mode 100644 src/main/java/nextflow/lsp/services/script/dag/Graph.java create mode 100644 src/main/java/nextflow/lsp/services/script/dag/VariableContext.java create mode 100644 src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy diff --git a/src/main/java/nextflow/lsp/NextflowLanguageServer.java b/src/main/java/nextflow/lsp/NextflowLanguageServer.java index 91c3bcd..84562c1 100644 --- a/src/main/java/nextflow/lsp/NextflowLanguageServer.java +++ b/src/main/java/nextflow/lsp/NextflowLanguageServer.java @@ -306,7 +306,7 @@ public CompletableFuture> codeLens(CodeLensParams param var service = getLanguageService(uri); if( service == null ) return Collections.emptyList(); - return service.codeLens(params, configuration.showVariablesInDAG()); + return service.codeLens(params); }); } @@ -457,8 +457,7 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) { withDefault(JsonUtils.getBoolean(settings, "nextflow.formatting.maheshForm"), configuration.maheshForm()), withDefault(JsonUtils.getInteger(settings, "nextflow.completion.maxItems"), configuration.maxCompletionItems()), withDefault(JsonUtils.getBoolean(settings, "nextflow.formatting.sortDeclarations"), configuration.sortDeclarations()), - withDefault(JsonUtils.getBoolean(settings, "nextflow.typeChecking"), configuration.typeChecking()), - withDefault(JsonUtils.getBoolean(settings, "nextflow.dag.showVariables"), configuration.showVariablesInDAG()) + withDefault(JsonUtils.getBoolean(settings, "nextflow.typeChecking"), configuration.typeChecking()) ); if( shouldInitialize(oldConfiguration, configuration) ) @@ -548,9 +547,8 @@ public CompletableFuture executeCommand(ExecuteCommandParams params) { return CompletableFutures.computeAsync((cancelChecker) -> { cancelChecker.checkCanceled(); var command = params.getCommand(); - List arguments = params.getArguments(); + var arguments = params.getArguments(); if( "nextflow.server.previewDag".equals(command) && arguments.size() == 2 ) { - arguments.add(configuration.showVariablesInDAG()); log.debug(String.format("textDocument/previewDag %s", arguments.toString())); var uri = JsonUtils.getString(arguments.get(0)); var service = getLanguageService(uri); diff --git a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java index d458f26..2c603ed 100644 --- a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java +++ b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java @@ -26,8 +26,7 @@ public record LanguageServerConfiguration( boolean maheshForm, int maxCompletionItems, boolean sortDeclarations, - boolean typeChecking, - boolean showVariablesInDAG + boolean typeChecking ) { public static LanguageServerConfiguration defaults() { @@ -39,7 +38,6 @@ public static LanguageServerConfiguration defaults() { false, 100, false, - false, false ); } diff --git a/src/main/java/nextflow/lsp/services/LanguageService.java b/src/main/java/nextflow/lsp/services/LanguageService.java index 4edbb76..bf352e0 100644 --- a/src/main/java/nextflow/lsp/services/LanguageService.java +++ b/src/main/java/nextflow/lsp/services/LanguageService.java @@ -109,7 +109,7 @@ public LanguageService(String rootUri) { public abstract boolean matchesFile(String uri); protected abstract ASTNodeCache getAstCache(); protected CallHierarchyProvider getCallHierarchyProvider() { return null; } - protected CodeLensProvider getCodeLensProvider(boolean showVariablesInDAG) { return null; } + protected CodeLensProvider getCodeLensProvider() { return null; } protected CompletionProvider getCompletionProvider(int maxItems, boolean extended) { return null; } protected DefinitionProvider getDefinitionProvider() { return null; } protected FormattingProvider getFormattingProvider() { return null; } @@ -186,8 +186,8 @@ public List callHierarchyOutgoingCalls(CallHierarchyI return provider.outgoingCalls(item); } - public List codeLens(CodeLensParams params, boolean showVariablesInDAG) { - var provider = getCodeLensProvider(showVariablesInDAG); + public List codeLens(CodeLensParams params) { + var provider = getCodeLensProvider(); if( provider == null ) return Collections.emptyList(); diff --git a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java index f163306..02745a3 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java @@ -41,11 +41,9 @@ public class ScriptCodeLensProvider implements CodeLensProvider { private static Logger log = Logger.getInstance(); private ScriptAstCache ast; - private boolean showVariablesInDAG; - public ScriptCodeLensProvider(ScriptAstCache ast, boolean showVariablesInDAG) { + public ScriptCodeLensProvider(ScriptAstCache ast) { this.ast = ast; - this.showVariablesInDAG = showVariablesInDAG; } @Override @@ -75,7 +73,7 @@ public List codeLens(TextDocumentIdentifier textDocument) { public Map previewDag(String documentUri, String name) { var uri = URI.create(documentUri); if( !ast.hasAST(uri) || ast.hasErrors(uri) ) - return Map.ofEntries(Map.entry("error", "DAG preview cannot be shown because the script has errors.")); + return Map.of("error", "DAG preview cannot be shown because the script has errors."); var sourceUnit = ast.getSourceUnit(uri); return ast.getWorkflowNodes(uri).stream() @@ -86,9 +84,9 @@ public Map previewDag(String documentUri, String name) { visitor.visit(); var graph = visitor.getGraph(wn.isEntry() ? "" : wn.getName()); - var result = new MermaidRenderer(!showVariablesInDAG).render(wn.getName(), graph); + var result = new MermaidRenderer().render(wn.getName(), graph); log.debug(result); - return Map.ofEntries(Map.entry("result", result)); + return Map.of("result", result); }) .orElse(null); } diff --git a/src/main/java/nextflow/lsp/services/script/ScriptService.java b/src/main/java/nextflow/lsp/services/script/ScriptService.java index be26daf..a4ead65 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptService.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptService.java @@ -71,8 +71,8 @@ protected CallHierarchyProvider getCallHierarchyProvider() { } @Override - protected CodeLensProvider getCodeLensProvider(boolean showVariablesInDAG) { - return new ScriptCodeLensProvider(astCache, showVariablesInDAG); + protected CodeLensProvider getCodeLensProvider() { + return new ScriptCodeLensProvider(astCache); } @Override @@ -123,11 +123,10 @@ protected SymbolProvider getSymbolProvider() { @Override public Object executeCommand(String command, List arguments) { updateNow(); - if( "nextflow.server.previewDag".equals(command) && arguments.size() == 3 ) { + if( "nextflow.server.previewDag".equals(command) && arguments.size() == 2 ) { var uri = getJsonString(arguments.get(0)); var name = getJsonString(arguments.get(1)); - boolean showVariablesInDAG = (boolean)arguments.get(2); - var provider = new ScriptCodeLensProvider(astCache, showVariablesInDAG); + var provider = new ScriptCodeLensProvider(astCache); return provider.previewDag(uri, name); } if( "nextflow.server.previewWorkspace".equals(command) ) { diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index 0f4f629..bcf772e 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -1,32 +1,30 @@ /* * Copyright 2024-2025, Seqera Labs * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except - * in compliance with the License. You may obtain a copy of the License at + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software distributed under the License - * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express - * or implied. See the License for the specific language governing permissions and limitations under - * the License. + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ package nextflow.lsp.services.script.dag; -import java.net.URI; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.Stack; -import java.util.function.Function; +import java.util.stream.Collectors; -import groovy.lang.Tuple2; import groovy.lang.Tuple3; import nextflow.lsp.services.script.ScriptAstCache; import nextflow.script.ast.ASTNodeMarker; @@ -50,76 +48,23 @@ * @author Ben Sherman * @author Erik Danielsson */ - -class Variable { - private int definitionDepth; - private Set activeInstances; - private Node.Type type; - - public Variable(Node activeInstance, int definitionDepth) { - this.activeInstances = new HashSet<>(); - activeInstances.add(activeInstance); - this.definitionDepth = definitionDepth; - type = activeInstance.type; - } - - private Variable(Set activeInstances, int definitionDepth) { - this.activeInstances = new HashSet<>(activeInstances); - this.definitionDepth = definitionDepth; - } - - public Node.Type getType() { - return type; - } - - public Variable shallowCopy() { - return new Variable(activeInstances, definitionDepth); - } - - public boolean isLocal(int currDepth) { - return definitionDepth == currDepth; - } - - public int getDefinitionDepth() { - return definitionDepth; - } - - public Set getActiveInstances() { - return activeInstances; - } - - public void setActiveInstance(Node dn) { - activeInstances.clear(); - activeInstances.add(dn); - } - - public Variable union(Variable other) { - var unionInstances = new HashSet<>(activeInstances); - unionInstances.addAll(other.getActiveInstances()); - return new Variable(unionInstances, definitionDepth); - } -} - public class DataflowVisitor extends ScriptVisitorSupport { private SourceUnit sourceUnit; private ScriptAstCache ast; - private Map graphs = new HashMap<>(); + private Map graphs = new HashMap<>(); private Stack> stackPreds = new Stack<>(); - private Stack> conditionalScopes = new Stack<>(); - - private int currentDepth = 0; + private VariableContext vc = new VariableContext(); public DataflowVisitor(SourceUnit sourceUnit, ScriptAstCache ast) { this.sourceUnit = sourceUnit; this.ast = ast; stackPreds.add(new HashSet<>()); - conditionalScopes.push(new HashMap<>()); } @Override @@ -147,107 +92,6 @@ public Graph getGraph(String name) { private boolean inEntry; - public Set getSymbolConditionalScope(String name) { - // get a variable node from the name table - var scope = conditionalScopes.peek(); - if( scope.containsKey(name) ) { - return scope.get(name).getActiveInstances(); - } else { - return null; - } - } - - public void putSymbolConditionalScope(String name, Node dn, boolean isLocal) { - - // Put a symbol into the currently active symbol table - var scope = conditionalScopes.peek(); - - if( scope.containsKey(name) ) { - - // We have reassinged the variable, which means that all previous - // definitions of it (in this scope) are dead. We can reuse the set - // though - Variable variable = scope.get(name); - variable.setActiveInstance(dn); - - } else { - - // The symbols in not present from before so create an empty set - Variable variable = new Variable(dn, isLocal ? currentDepth : 0); - scope.put(name, variable); - - } - } - - public void pushConditionalScope(Map previousSymbols) { - - // We want to add all symbols in the current scope - // since they always enter both branches - var newScope = new HashMap(); - - for( Map.Entry entry : previousSymbols.entrySet() ) { - newScope.put(entry.getKey(), entry.getValue().shallowCopy()); - } - - conditionalScopes.push(newScope); - } - - public Map popConditionalScope() { - return conditionalScopes.pop(); - } - - public Map mergeConditionalScopes(Map ifSymbols, - Map elseSymbols, Subgraph ifSubgraph, Subgraph elseSubgraph) { - // Merge the set nodes corresponding to active symbols for the two scopes - var merged = new HashMap(); - // Add all keys from ifSymbols - for( Map.Entry entry : ifSymbols.entrySet() ) { - String key = entry.getKey(); - Variable ifVariable = entry.getValue(); - if( ifVariable.getDefinitionDepth() <= currentDepth ) { - if( elseSymbols.containsKey(key) ) { - merged.put(key, ifVariable.union(elseSymbols.get(key))); - } else { - // The variable is only found in the else branch, so we should add a null - // initializer to it - if( ifVariable.getType() == Node.Type.NAME ) { - Node uninitNode = fixUninitNodeSubgraph(key, elseSubgraph); - merged.put(key, ifVariable.union(new Variable(uninitNode, 0))); - } else { - merged.put(key, ifVariable.shallowCopy()); - } - } - } - } - - // Add remaining keys from elseSymbols (already handled common ones) - for( Map.Entry entry : elseSymbols.entrySet() ) { - String key = entry.getKey(); - Variable elseVariable = entry.getValue(); - if( !merged.containsKey(key) && elseVariable.getDefinitionDepth() <= currentDepth ) { - // The variable is only found in the if branch, so we should add a null - // initializer to it - if( elseVariable.getType() == Node.Type.NAME ) { - Node uninitNode = fixUninitNodeSubgraph(key, ifSubgraph); - merged.put(key, elseVariable.union(new Variable(uninitNode, 0))); - } else { - merged.put(key, elseVariable.shallowCopy()); - } - } - } - - return merged; - } - - private Node fixUninitNodeSubgraph(String label, Subgraph subgraph) { - var nullAndUninitNode = addUninitalizedNode(label, null); - var nullNode = nullAndUninitNode.getV1(); - var uninitNode = nullAndUninitNode.getV2(); - subgraph.addMember(nullNode); - subgraph.addMember(uninitNode); - return uninitNode; - } - @Override public void visitWorkflow(WorkflowNode node) { current = new Graph(); @@ -264,42 +108,39 @@ public void visitWorkflow(WorkflowNode node) { current = null; } - private void visitWorkflowTakes(WorkflowNode node, Map result) { + private void visitWorkflowTakes(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.takes) ) { var name = asVarX(stmt).getName(); var dn = addNode(name, Node.Type.NAME, stmt); - putSymbolConditionalScope(name, dn, false); + vc.putSymbol(name, dn); result.put(name, dn); } } - private void visitWorkflowEmits(WorkflowNode node, Map result) { + private void visitWorkflowEmits(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.emits) ) { var emit = ((ExpressionStatement) stmt).getExpression(); String name; if( emit instanceof VariableExpression ve ) { name = ve.getName(); - } else if( emit instanceof AssignmentExpression assign ) { + } + else if( emit instanceof AssignmentExpression assign ) { var target = (VariableExpression) assign.getLeftExpression(); name = target.getName(); visit(emit); - } else { + } + else { name = "$out"; visit(new AssignmentExpression(varX(name), emit)); } - var dns = getSymbolConditionalScope(name); - if( dns == null ) { + var dn = getSymbol(name); + if( dn == null ) System.err.println("missing emit: " + name); - result.put(name, null); - } else if( dns.size() > 1 ) { - System.err.println("two many emits: " + name); - } else { - result.put(name, dns.iterator().next()); - } + result.put(name, dn); } } - private void visitWorkflowPublishers(WorkflowNode node, Map result) { + private void visitWorkflowPublishers(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.publishers) ) { var es = (ExpressionStatement) stmt; var publisher = (BinaryExpression) es.getExpression(); @@ -307,13 +148,50 @@ private void visitWorkflowPublishers(WorkflowNode node, Map result var source = publisher.getRightExpression(); visit(new AssignmentExpression(target, source)); var name = target.getName(); - var dns = getSymbolConditionalScope(name); - if( dns.size() == 1 ) { - result.put(name, dns.iterator().next()); - } + var dn = getSymbol(name); + if( dn == null ) + System.err.println("missing publisher: " + name); + result.put(name, dn); } } + // statements + + @Override + public void visitIfElse(IfStatement node) { + // visit the conditional expression + var preds = visitWithPreds(node.getBooleanExpression()); + var controlDn = addNode("", Node.Type.CONTROL, null, preds); + + // visit the if branch + vc.pushScope(); + current.pushSubgraph(controlDn); + visitWithPreds(node.getIfBlock()); + + var ifScope = vc.popScope(); + var ifSubgraph = current.popSubgraph(); + + // visit the else branch + Map elseScope; + + if( !node.getElseBlock().isEmpty() ) { + vc.pushScope(); + current.pushSubgraph(controlDn); + visitWithPreds(node.getElseBlock()); + + elseScope = vc.popScope(); + current.popSubgraph(); + } + else { + // if there is no else branch, then the set of active symbols + // after the if statement is the union of the active symbols + // from before the if and the active symbols in the if + elseScope = vc.peekScope(); + } + + vc.mergeConditionalScopes(ifScope, elseScope); + } + // expressions @Override @@ -334,7 +212,7 @@ public void visitMethodCallExpression(MethodCallExpression node) { if( defNode instanceof WorkflowNode || defNode instanceof ProcessNode ) { var preds = visitWithPreds(node.getArguments()); var dn = addNode(name, Node.Type.OPERATOR, defNode, preds); - putSymbolConditionalScope(name, dn, false); + vc.putSymbol(name, dn); return; } @@ -355,103 +233,25 @@ public void visitBinaryExpression(BinaryExpression node) { super.visitBinaryExpression(node); } - @Override - public void visitIfElse(IfStatement node) { - - // First visit the conditional - var preds = visitWithPreds(node.getBooleanExpression()); - var conditionalDn = addNode("", Node.Type.CONDITIONAL, node, preds); - - // Then construct new symbol tables for each of the branches - // Get the symbol table associated with the currently available global - // variables - var precedingScope = popConditionalScope(); - - // Construct the scope for the if branch - pushConditionalScope(precedingScope); - currentDepth += 1; - - current.pushSubgraph(); - current.putSubgraphPred(conditionalDn); - - visitWithPreds(node.getIfBlock()); - - var ifScope = popConditionalScope(); - var ifSubgraph = current.popSubgraph(); - - Map elseScope; - Subgraph elseSubgraph; - if( !node.getElseBlock().isEmpty() ) { - - // We push a new symbol table to keep track of the - // symbols in the else branch. - pushConditionalScope(precedingScope); - - // Push a new subgraph to keep track of the else branch - current.pushSubgraph(); - current.putSubgraphPred(conditionalDn); - visitWithPreds(node.getElseBlock()); - - // Exit the else branch - elseScope = popConditionalScope(); - elseSubgraph = current.popSubgraph(); - - } else { - - // If there is only an if branch then the active symbols after the statement - // is the union of the active symbols from before the if and the active symbols - // in the if - elseScope = precedingScope; - elseSubgraph = current.peekSubgraph(); - + private void visitAssignment(BinaryExpression node, boolean isLocal) { + var preds = visitWithPreds(node.getRightExpression()); + var targets = getAssignmentTargets(node.getLeftExpression()); + for( var name : targets ) { + var dn = addNode(name, Node.Type.NAME, null, preds); + vc.putSymbol(name, dn, isLocal); } - - currentDepth -= 1; - var succedingScope = mergeConditionalScopes(ifScope, elseScope, ifSubgraph, elseSubgraph); - pushConditionalScope(succedingScope); - } - private void visitAssignment(BinaryExpression node, boolean isDef) { - - var targetExpr = node.getLeftExpression(); - var sourceExpr = node.getRightExpression(); - - if( targetExpr instanceof TupleExpression tupleExpr && sourceExpr instanceof ListExpression listExpr ) { - - // e.g. (x, y, z) = [1, 2, 3]. - // We need to keep track what variable is assigned where - List targetExprList = tupleExpr.getExpressions(); - List sourceExprList = listExpr.getExpressions(); - - // We need to wait with adding the nodes, so that we do not overwrite live - // variable names in the source expression - List>> namesAndPreds = new ArrayList<>(); - - for( int i = 0; i < Math.min(targetExprList.size(), sourceExprList.size()); i++ ) { - String name = getAssignmentTarget(targetExprList.get(i)).getName(); - var preds = visitWithPreds(sourceExprList.get(i)); - namesAndPreds.add(new Tuple2<>(name, preds)); - } - - for( var namePreds : namesAndPreds ) { - var name = namePreds.getV1(); - var preds = namePreds.getV2(); - var dn = addNode(name, Node.Type.NAME, null, preds); - putSymbolConditionalScope(name, dn, isDef); - } - } else { - - processAssignment(targetExpr, sourceExpr, isDef); - + private Set getAssignmentTargets(Expression node) { + // e.g. (x, y, z) = xyz + if( node instanceof TupleExpression te ) { + return te.getExpressions().stream() + .map(el -> getAssignmentTarget(el).getName()) + .collect(Collectors.toSet()); + } + else { + return Set.of(getAssignmentTarget(node).getName()); } - } - - private void processAssignment(Expression target, Expression source, boolean isDef) { - String name = getAssignmentTarget(target).getName(); - var preds = visitWithPreds(source); - var dn = addNode(name, Node.Type.NAME, null, preds); - putSymbolConditionalScope(name, dn, isDef); } private VariableExpression getAssignmentTarget(Expression node) { @@ -478,7 +278,7 @@ private void visitPipeline(BinaryExpression node) { var label = defNode.getName(); var preds = visitWithPreds(lhs); var dn = addNode(label, Node.Type.OPERATOR, defNode, preds); - putSymbolConditionalScope(label, dn, false); + vc.putSymbol(label, dn); return; } } @@ -493,6 +293,7 @@ public void visitDeclarationExpression(DeclarationExpression node) { @Override public void visitClosureExpression(ClosureExpression node) { + // skip closures since they can't contain dataflow logic } @Override @@ -528,21 +329,20 @@ private boolean isEntryParam(PropertyExpression node) { return inEntry && node.getObjectExpression() instanceof VariableExpression ve && "params".equals(ve.getName()); } - private Tuple3 asMethodOutput(PropertyExpression node) { + private Tuple3 asMethodOutput(PropertyExpression node) { // named output e.g. PROC.out.foo if( node.getObjectExpression() instanceof PropertyExpression pe ) { if( pe.getObjectExpression() instanceof VariableExpression ve && "out".equals(pe.getPropertyAsString()) ) { var mn = asMethodVariable(ve.getAccessedVariable()); if( mn != null ) - return new Tuple3(mn, ve.getName(), node.getPropertyAsString()); + return new Tuple3(mn, ve.getName(), node.getPropertyAsString()); } } // single output e.g. PROC.out - else if( node.getObjectExpression() instanceof VariableExpression ve - && "out".equals(node.getPropertyAsString()) ) { + else if( node.getObjectExpression() instanceof VariableExpression ve && "out".equals(node.getPropertyAsString()) ) { var mn = asMethodVariable(ve.getAccessedVariable()); if( mn != null ) - return new Tuple3(mn, ve.getName(), null); + return new Tuple3(mn, ve.getName(), null); } return null; } @@ -552,21 +352,29 @@ private void visitProcessOut(ProcessNode process, String label, String propName) addOperatorPred(label, process); return; } - asDirectives(process.outputs).filter((call) -> { - var emitName = getProcessEmitName(call); - return propName.equals(emitName); - }).findFirst().ifPresent((call) -> { - addOperatorPred(label, process); - }); + asDirectives(process.outputs) + .filter((call) -> { + var emitName = getProcessEmitName(call); + return propName.equals(emitName); + }) + .findFirst() + .ifPresent((call) -> { + addOperatorPred(label, process); + }); } private String getProcessEmitName(MethodCallExpression output) { - return Optional.of(output).flatMap(call -> Optional.ofNullable(asNamedArgs(call))) - .flatMap(namedArgs -> namedArgs.stream() - .filter(entry -> "emit".equals(entry.getKeyExpression().getText())).findFirst()) - .flatMap(entry -> Optional - .ofNullable(entry.getValueExpression() instanceof VariableExpression ve ? ve.getName() : null)) - .orElse(null); + return Optional.of(output) + .flatMap(call -> Optional.ofNullable(asNamedArgs(call))) + .flatMap(namedArgs -> + namedArgs.stream() + .filter(entry -> "emit".equals(entry.getKeyExpression().getText())) + .findFirst() + ) + .flatMap(entry -> Optional.ofNullable( + entry.getValueExpression() instanceof VariableExpression ve ? ve.getName() : null + )) + .orElse(null); } private void visitWorkflowOut(WorkflowNode workflow, String label, String propName) { @@ -574,19 +382,23 @@ private void visitWorkflowOut(WorkflowNode workflow, String label, String propNa addOperatorPred(label, workflow); return; } - asBlockStatements(workflow.emits).stream().map(stmt -> ((ExpressionStatement) stmt).getExpression()) - .filter((emit) -> { - var emitName = getWorkflowEmitName(emit); - return propName.equals(emitName); - }).findFirst().ifPresent((call) -> { - addOperatorPred(label, workflow); - }); + asBlockStatements(workflow.emits).stream() + .map(stmt -> ((ExpressionStatement) stmt).getExpression()) + .filter((emit) -> { + var emitName = getWorkflowEmitName(emit); + return propName.equals(emitName); + }) + .findFirst() + .ifPresent((call) -> { + addOperatorPred(label, workflow); + }); } private String getWorkflowEmitName(Expression emit) { if( emit instanceof VariableExpression ve ) { return ve.getName(); - } else if( emit instanceof AssignmentExpression assign ) { + } + else if( emit instanceof AssignmentExpression assign ) { var target = (VariableExpression) assign.getLeftExpression(); return target.getName(); } @@ -594,29 +406,32 @@ private String getWorkflowEmitName(Expression emit) { } private void addOperatorPred(String label, ASTNode an) { - Set dns = getSymbolConditionalScope(label); - if( dns != null ) { - for( var dn : dns ) { - currentPreds().add(dn); - } - } else { - var dn = addNode(label, Node.Type.OPERATOR, an); - putSymbolConditionalScope(label, dn, false); - } + var dn = getSymbol(label); + if( dn != null ) + currentPreds().add(dn); + else + vc.putSymbol(label, addNode(label, Node.Type.OPERATOR, an)); } @Override public void visitVariableExpression(VariableExpression node) { var name = node.getName(); - Set dns = getSymbolConditionalScope(name); - if( dns != null ) - for( Node dn : dns ) { - currentPreds().add(dn); - } + var dn = getSymbol(name); + if( dn != null ) + currentPreds().add(dn); } // helpers + private Node getSymbol(String name) { + var preds = vc.getSymbol(name); + if( preds.isEmpty() ) + return null; + if( preds.size() == 1 ) + return preds.iterator().next(); + return addNode(name, Node.Type.NAME, null, preds); + } + private Set currentPreds() { return stackPreds.peek(); } @@ -645,206 +460,7 @@ private Node addNode(String label, Node.Type type, ASTNode an, Set preds) } private Node addNode(String label, Node.Type type, ASTNode an) { - var dn = addNode(label, type, an, new HashSet<>()); - return dn; - } - - private Tuple2 addUninitalizedNode(String label, ASTNode an) { - var nullNode = addNode("null", Node.Type.NULL, an); - var preds = new HashSet(); - preds.add(nullNode); - var uninitalizedNode = current.addNode(label, Node.Type.NAME, null, preds); - return new Tuple2<>(nullNode, uninitalizedNode); - } - -} - -class Graph { - - public final Map inputs = new HashMap<>(); - - public final Map nodes = new HashMap<>(); - - public final Map outputs = new HashMap<>(); - - private Stack> scopes = new Stack<>(); - - public Stack activeSubgraphs = new Stack<>(); - - private int currSubgraphId = 0; - - public Graph() { - pushScope(); - pushSubgraph(); - } - - public void pushScope() { - scopes.add(0, new HashMap<>()); - } - - public void popScope() { - scopes.remove(0); - } - - public void pushSubgraph() { - activeSubgraphs.push(new Subgraph(currSubgraphId)); - currSubgraphId += 1; - } - - public Subgraph popSubgraph() { - var prevSubgraph = activeSubgraphs.pop(); - var currSubgraph = activeSubgraphs.peek(); - currSubgraph.addChild(prevSubgraph); - return prevSubgraph; - } - - public Subgraph peekSubgraph() { - return activeSubgraphs.peek(); - } - - public void putToSubgraph(Node n) { - activeSubgraphs.peek().addMember(n); + return addNode(label, type, an, new HashSet<>()); } - public void putSubgraphPred(Node n) { - activeSubgraphs.peek().addPred(n); - } - - public Node addNode(String label, Node.Type type, URI uri, Set preds) { - var id = nodes.size(); - var dn = new Node(id, label, type, uri, preds); - nodes.put(id, dn); - putToSubgraph(dn); - return dn; - } - - public void collapseGraph(Function hide, Function hideIfDisconnected) { - // We want to collapse the graph by removing all nodes satisfying the predicate `isHidden` - // and then look for disconnected nodes which should be hidden if they satisfy `hideIfDisconnected` - Set visited = new HashSet<>(); - Set remove = new HashSet<>(); - Set maybeRemove = new HashSet<>(); - for (Map.Entry idNode : nodes.entrySet()) { - int id = idNode.getKey(); - Node node = idNode.getValue(); - if (!hide.apply(node)) { - if (!hideIfDisconnected.apply(node)) { - // We have found a node that should always be connected, start searching from here - findTrueAncestors(node, hide, visited); - visited.add(id); - } else { - maybeRemove.add(id); - } - } else { - remove.add(id); - } - } - // If a node was visited then it is necessarily connected to a node we are keeping - maybeRemove.removeAll(visited); - remove.addAll(maybeRemove); - - for (int id : remove) - nodes.remove(id); - - } - - private Set findTrueAncestors(Node node, Function hide, Set visited) { - Set truePreds = new HashSet<>(); - for (Node pred : node.preds) { - if (!hide.apply(pred)) { - truePreds.add(pred); - } else if (!visited.contains(pred.id)) { - var predAncestors = findTrueAncestors(pred, hide, visited); - truePreds.addAll(predAncestors); - } else { - // We have already visited the node and have found its ancestors - truePreds.addAll(pred.preds); - } - visited.add(pred.id); - } - node.preds.clear(); - node.preds.addAll(truePreds); - return truePreds; - } - -} -class Subgraph { - - private int id; - - private List children = new ArrayList<>(); - - private Set members = new HashSet<>(); - - private Set preds = new HashSet<>(); - - public Subgraph(int id) { - this.id = id; - } - - public int getId() { - return id; - } - - public void addMember(Node n) { - members.add(n); - } - - public Set getMembers() { - return members; - } - - public void addChild(Subgraph s) { - children.add(s); - } - - public List getChildren() { - return children; - } - - public Set getPreds() { - return preds; - } - - public void addPred(Node n) { - preds.add(n); - } -} -class Node { - public enum Type { - NAME, OPERATOR, CONDITIONAL, NULL - } - - public final int id; - public final String label; - public final Type type; - public final URI uri; - public final Set preds; - - public Node(int id, String label, Type type, URI uri, Set preds) { - this.id = id; - this.label = label; - this.type = type; - this.uri = uri; - this.preds = preds; - } - - public void addPredecessors(Set preds) { - this.preds.addAll(preds); - } - - @Override - public boolean equals(Object other) { - return other instanceof Node n && this.id == n.id; - } - - @Override - public int hashCode() { - return id; - } - - @Override - public String toString() { - return String.format("id=%s,label='%s',type=%s", id, label, type); - } } diff --git a/src/main/java/nextflow/lsp/services/script/dag/Graph.java b/src/main/java/nextflow/lsp/services/script/dag/Graph.java new file mode 100644 index 0000000..14e3bfa --- /dev/null +++ b/src/main/java/nextflow/lsp/services/script/dag/Graph.java @@ -0,0 +1,135 @@ +/* + * Copyright 2024-2025, Seqera Labs + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package nextflow.lsp.services.script.dag; + +import java.net.URI; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.Stack; + +/** + * + * @author Ben Sherman + * @author Erik Danielsson + */ +class Graph { + + public final Map inputs = new HashMap<>(); + + public final Map nodes = new HashMap<>(); + + public final Map outputs = new HashMap<>(); + + private Stack subgraphs = new Stack<>(); + + private int nextSubgraphId = 0; + + public Graph() { + pushSubgraph(); + } + + public Subgraph peekSubgraph() { + return subgraphs.peek(); + } + + public void pushSubgraph() { + subgraphs.push(new Subgraph(nextSubgraphId, Collections.emptySet())); + nextSubgraphId += 1; + } + + public void pushSubgraph(Node dn) { + subgraphs.push(new Subgraph(nextSubgraphId, Set.of(dn))); + nextSubgraphId += 1; + } + + public Subgraph popSubgraph() { + var result = subgraphs.pop(); + subgraphs.peek().subgraphs.add(result); + return result; + } + + public Node addNode(String label, Node.Type type, URI uri, Set preds) { + var id = nodes.size(); + var dn = new Node(id, label, type, uri, preds); + nodes.put(id, dn); + subgraphs.peek().nodes.add(dn); + return dn; + } +} + + +class Subgraph { + + public final int id; + + public final Set preds; + + public final List subgraphs = new ArrayList<>(); + + public final Set nodes = new HashSet<>(); + + public Subgraph(int id, Set preds) { + this.id = id; + this.preds = preds; + } +} + + +class Node { + public enum Type { + NAME, + OPERATOR, + CONTROL + } + + public final int id; + public final String label; + public final Type type; + public final URI uri; + public final Set preds; + + public Node(int id, String label, Type type, URI uri, Set preds) { + this.id = id; + this.label = label; + this.type = type; + this.uri = uri; + this.preds = preds; + } + + public void addPredecessors(Set preds) { + this.preds.addAll(preds); + } + + @Override + public boolean equals(Object other) { + return other instanceof Node n && this.id == n.id; + } + + @Override + public int hashCode() { + return id; + } + + @Override + public String toString() { + return String.format("id=%s,label='%s',type=%s", id, label, type); + } +} diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 2890b2f..ca657e7 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -15,15 +15,11 @@ */ package nextflow.lsp.services.script.dag; - -import java.util.ArrayList; -import java.util.Set; import java.util.Collection; import java.util.HashSet; -import java.util.function.Function; - - -import groovy.lang.Tuple2; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * @@ -32,95 +28,144 @@ */ public class MermaidRenderer { - private final boolean hideVariables; - private final boolean uniqueNames; - private Collection inputs = null; - private Collection outputs = null; + private final boolean showVariables = false; + + private StringBuilder builder; + + private int indent; - public MermaidRenderer(boolean hideVariables) { - this.hideVariables = hideVariables; - this.uniqueNames = false; + private void append(String format, Object... args) { + builder.append(" ".repeat(indent)); + builder.append(String.format(format, args)); + builder.append('\n'); + } + + public void incIndent() { + indent++; + } + + public void decIndent() { + indent--; } public String render(String name, Graph graph) { - var isEntry = name == null; - var lines = new ArrayList(); - lines.add("flowchart TB"); - lines.add(String.format("subgraph %s", isEntry ? "\" \"" : name)); - // prepare inputs and outputs - inputs = graph.inputs.values(); + var isEntry = name == null; + var inputs = graph.inputs.values(); var nodes = graph.nodes.values(); - outputs = graph.outputs.values(); + var outputs = graph.outputs.values(); + + // render graph + builder = new StringBuilder(); + indent = 0; - // Collapse graph i.e. remove nodes that shouldn't be part of the layout - graph.collapseGraph(isHidden(inputs, outputs), isHiddenIfDisconnected()); + append("flowchart TB"); + incIndent(); + append("subgraph %s", isEntry ? "\" \"" : name); + incIndent(); // render inputs if( inputs.size() > 0 ) { - lines.add(String.format(" subgraph %s", isEntry ? "params" : "take")); + append("subgraph %s", isEntry ? "params" : "take"); + incIndent(); for( var dn : inputs ) { if( dn == null ) continue; - lines.add(" " + renderNode(dn.id, dn.label, dn.type)); + append(renderNode(dn.id, dn.label, dn.type)); } - lines.add(" end"); + decIndent(); + append("end"); } - var subgraphEdges = renderSubgraphs(graph.activeSubgraphs.peek(), graph, lines, 0); + // render nodes + var subgraphEdges = renderSubgraph(graph.peekSubgraph()); // render outputs if( outputs.size() > 0 ) { - lines.add(String.format(" subgraph %s", isEntry ? "publish" : "emit")); + append("subgraph %s", isEntry ? "publish" : "emit"); + incIndent(); for( var dn : outputs ) - lines.add(" " + renderNode(dn.id, dn.label, dn.type)); - lines.add(" end"); + append(renderNode(dn.id, dn.label, dn.type)); + decIndent(); + append("end"); } // render edges - for( var dn : nodes ) - for( var dnPred : dn.preds ) - lines.add(String.format(" v%d --> v%d", dnPred.id, dn.id)); + for( var dn : nodes ) { + if( isHidden(dn, inputs, outputs) ) + continue; + + var preds = dn.preds; + var visited = new HashSet(); + while( true ) { + var done = preds.stream().allMatch(p -> !isHidden(p, inputs, outputs)); + if( done ) + break; + visited.addAll(preds); + preds = preds.stream() + .flatMap(pred -> ( + isHidden(pred, inputs, outputs) + ? pred.preds.stream().filter(p -> !visited.contains(p)) + : Stream.of(pred) + )) + .collect(Collectors.toSet()); + } + + for( var dnPred : preds ) + append("v%d --> v%d", dnPred.id, dn.id); + } - for( var e : subgraphEdges ) { - lines.add(String.format(" v%d --> s%d", e.getV2(), e.getV1())); + for( var edge : subgraphEdges ) { + append("v%d --> s%d", edge.source(), edge.target()); } - lines.add("end"); + decIndent(); + append("end"); - return String.join("\n", lines); + return builder.toString(); } - // Write the subgraphs and fetch the list of subgraph edges - private Set> renderSubgraphs(Subgraph s, Graph g, ArrayList lines, int depth) { - - if( depth > 0 ) { - lines.add(" ".repeat(depth) + String.format("subgraph s%d[\" \"]", s.getId())); + /** + * Render a subgraph and collect all incident edges. + * + * @param subgraph + */ + private Set renderSubgraph(Subgraph subgraph) { + if( subgraph.id > 0 ) { + append("subgraph s%d[\" \"]", subgraph.id); + incIndent(); } - for( var dn : s.getMembers() ) { - if (g.nodes.containsKey(dn.id)) { - var label = dn.label.replaceAll("\n", "\\\\\n").replaceAll("\"", "\\\\\""); + // render nodes + for( var dn : subgraph.nodes ) { + if( dn.type == Node.Type.NAME ) + continue; - lines.add(" ".repeat(depth + 1) + renderNode(dn.id, label, dn.type)); - if( dn.uri != null ) { - lines.add(String.format(" click v%d href \"%s\" _blank", dn.id, dn.uri.toString())); - } + var label = dn.label + .replaceAll("\n", "\\\\\n") + .replaceAll("\"", "\\\\\""); - } + append(renderNode(dn.id, label, dn.type)); + if( dn.uri != null ) + append("click v%d href \"%s\" _blank", dn.id, dn.uri.toString()); } - // Get incoming edges - Set> incidentEdges = new HashSet<>(); - for( var p : s.getPreds() ) { - incidentEdges.add(new Tuple2(s.getId(), p.id)); + + // render subgraphs and collect incident edges + var incidentEdges = new HashSet(); + for( var dnPred : subgraph.preds ) { + incidentEdges.add(new Edge(dnPred.id, subgraph.id)); } - for( var child : s.getChildren() ) { - var moreEdges = renderSubgraphs(child, g, lines, depth + 1); - incidentEdges.addAll(moreEdges); + + for( var s : subgraph.subgraphs ) { + var edges = renderSubgraph(s); + incidentEdges.addAll(edges); } - if( depth > 0 ) { - lines.add(" ".repeat(depth) + "end"); + + if( subgraph.id > 0 ) { + decIndent(); + append("end"); } + return incidentEdges; } @@ -131,27 +176,25 @@ private Set> renderSubgraphs(Subgraph s, Graph g, Array * @param inputs * @param outputs */ - private Function isHidden(Collection inputs, Collection outputs) { - return (dn -> hideVariables && dn.type == Node.Type.NAME && !inputs.contains(dn) && !outputs.contains(dn)); + private boolean isHidden(Node dn, Collection inputs, Collection outputs) { + return isHidden(dn) && !inputs.contains(dn) && !outputs.contains(dn); } - private Function isHiddenIfDisconnected() { - return (dn -> hideVariables && dn.type == Node.Type.NULL); + private boolean isHidden(Node dn) { + return !showVariables && dn.type == Node.Type.NAME; } - private String renderNode(int id, String label, Node.Type type) { - String name; - if( uniqueNames ) { - name = String.format("%s<%d>", label, id); - } else { - name = String.format("%s", label); - } - return switch (type) { - case NAME -> String.format("v%d[\"%s\"]", id, name); - case OPERATOR -> String.format("v%d([%s])", id, name); - case CONDITIONAL -> String.format("v%d([conditional])", id); - case NULL -> String.format("v%d{null}", id); + private static String renderNode(int id, String label, Node.Type type) { + return switch( type ) { + case NAME -> String.format("v%d[\"%s\"]", id, label); + case OPERATOR -> String.format("v%d([%s])", id, label); + case CONTROL -> String.format("v%d{ }", id); }; } + private static record Edge( + int source, + int target + ) {} + } diff --git a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java new file mode 100644 index 0000000..01b0270 --- /dev/null +++ b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java @@ -0,0 +1,168 @@ +/* + * Copyright 2024-2025, Seqera Labs + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package nextflow.lsp.services.script.dag; + +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.Stack; + +/** + * + * @author Ben Sherman + * @author Erik Danielsson + */ +class VariableContext { + + private Stack> scopes = new Stack<>(); + + public VariableContext() { + scopes.push(new HashMap<>()); + } + + /** + * Get the current scope. + */ + public Map peekScope() { + return scopes.peek(); + } + + /** + * Enter a new scope, inheriting all symbols defined + * in the parent scope. + */ + public void pushScope() { + var newScope = new HashMap(); + scopes.peek().forEach((name, variable) -> { + newScope.put(name, variable.shallowCopy()); + }); + scopes.push(newScope); + } + + /** + * Exit the current scope. + */ + public Map popScope() { + return scopes.pop(); + } + + /** + * Get the current predecessors for a given symbol. + * + * @param name + */ + public Set getSymbol(String name) { + var scope = scopes.peek(); + return scope.containsKey(name) + ? scope.get(name).preds + : Collections.emptySet(); + } + + /** + * Put a symbol into the current scope. + * + * @param name + * @param dn + * @param isLocal + */ + public void putSymbol(String name, Node dn, boolean isLocal) { + var scope = scopes.peek(); + if( scope.containsKey(name) ) { + // reassign variable if it is already defined + var variable = scope.get(name); + variable.preds.clear(); + variable.preds.add(dn); + } + else { + var depth = isLocal ? currentDepth() : 1; + var preds = new HashSet(); + preds.add(dn); + var variable = new Variable(depth, preds); + scope.put(name, variable); + } + } + + public void putSymbol(String name, Node dn) { + putSymbol(name, dn, false); + } + + /** + * Merge two conditional scopes into the current scope. + * + * @param ifScope + * @param elseScope + */ + public void mergeConditionalScopes(Map ifScope, Map elseScope) { + var allSymbols = new HashMap(); + + // add symbols from if branch + ifScope.forEach((name, variable) -> { + if( variable.depth > currentDepth() ) + return; + + var other = elseScope.get(name); + if( other != null ) + variable = variable.union(other); + + if( allSymbols.containsKey(name) ) + allSymbols.put(name, allSymbols.get(name).union(variable)); + else + allSymbols.put(name, variable.shallowCopy()); + }); + + // add remaining symbols from else branch + elseScope.forEach((name, variable) -> { + if( variable.depth > currentDepth() ) + return; + + if( !allSymbols.containsKey(name) ) + allSymbols.put(name, variable.shallowCopy()); + }); + + // add merged symbols to current scope + scopes.peek().putAll(allSymbols); + } + + private int currentDepth() { + return scopes.size(); + } + +} + + +class Variable { + + public final int depth; + + public final Set preds; + + Variable(int depth, Set preds) { + this.depth = depth; + this.preds = preds; + } + + public Variable shallowCopy() { + return new Variable(depth, new HashSet(preds)); + } + + public Variable union(Variable other) { + var allPreds = new HashSet(preds); + allPreds.addAll(other.preds); + return new Variable(depth, allPreds); + } +} diff --git a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy new file mode 100644 index 0000000..ad7e0fe --- /dev/null +++ b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy @@ -0,0 +1,168 @@ +/* + * Copyright 2024-2025, Seqera Labs + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package nextflow.lsp.services.script.dag + +import nextflow.lsp.services.script.ScriptService +import spock.lang.Specification + +import static nextflow.lsp.TestUtils.* +import static nextflow.lsp.util.JsonUtils.* + +/** + * + * @author Ben Sherman + */ +class PreviewDagTest extends Specification { + + boolean checkDagPreview(ScriptService service, String uri, String source, String mmd) { + open(service, uri, source.stripIndent()) + def response = service.executeCommand('nextflow.server.previewDag', [asJson(uri), asJson(null)]) + assert response.result == mmd.stripIndent() + return true + } + + def 'should render the DAG preview for a workflow' () { + given: + def service = getScriptService() + def uri = getUri('main.nf') + + expect: + checkDagPreview(service, uri, + '''\ + workflow { + if (params.echo) { + echo = TOUCH(params.echo) + } else { + echo = DEFAULT() + } + APPEND(echo) + } + + process TOUCH { + input: + val x + + script: + true + } + + process DEFAULT { + true + } + + process APPEND { + input: + val x + + script: + true + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v0["echo"] + end + v1{ } + v7([APPEND]) + click v7 href "$uri" _blank + subgraph s1[" "] + v2([TOUCH]) + click v2 href "$uri" _blank + end + subgraph s2[" "] + v4([DEFAULT]) + click v4 href "$uri" _blank + end + v0 --> v1 + v0 --> v2 + v2 --> v7 + v4 --> v7 + v1 --> s1 + v1 --> s2 + end + """ + ) + } + + def 'should handle variable reassignment in an if statement' () { + given: + def service = getScriptService() + def uri = getUri('main.nf') + + expect: + checkDagPreview(service, uri, + '''\ + workflow { + main: + ch_versions = channel.empty() + + FOO() + ch_versions = ch_versions.mix( FOO.out.versions ) + + if (params.bar) { + BAR() + ch_versions = ch_versions.mix( BAR.out.versions ) + } + + publish: + versions = ch_versions + } + + process FOO { + output: + val 'versions', emit: versions + + script: + true + } + + process BAR { + output: + val 'versions', emit: versions + + script: + true + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v3["bar"] + end + v1([FOO]) + click v1 href "$uri" _blank + v4{ } + subgraph s1[" "] + v5([BAR]) + click v5 href "$uri" _blank + end + subgraph publish + v8["versions"] + end + v3 --> v4 + v1 --> v8 + v5 --> v8 + v4 --> s1 + end + """ + ) + } + +} From c48a91e5310e4bbdf2a9f0379c8dc5ec614999f4 Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Wed, 30 Jul 2025 13:23:19 -0500 Subject: [PATCH 08/15] minor edits --- .../lsp/services/script/dag/MermaidRenderer.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index ca657e7..1f05642 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -34,17 +34,22 @@ public class MermaidRenderer { private int indent; + private void reset() { + builder = new StringBuilder(); + indent = 0; + } + private void append(String format, Object... args) { builder.append(" ".repeat(indent)); builder.append(String.format(format, args)); builder.append('\n'); } - public void incIndent() { + private void incIndent() { indent++; } - public void decIndent() { + private void decIndent() { indent--; } @@ -56,9 +61,7 @@ public String render(String name, Graph graph) { var outputs = graph.outputs.values(); // render graph - builder = new StringBuilder(); - indent = 0; - + reset(); append("flowchart TB"); incIndent(); append("subgraph %s", isEntry ? "\" \"" : name); From 8985078e0e7f3766a2207fc9a9633e3d8828fcff Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Wed, 30 Jul 2025 14:34:35 -0500 Subject: [PATCH 09/15] Support ternary expression --- .../services/script/dag/DataflowVisitor.java | 44 ++++++++++--- .../services/script/dag/MermaidRenderer.java | 31 ++++----- .../services/script/dag/PreviewDagTest.groovy | 65 ++++++++++++++++++- 3 files changed, 112 insertions(+), 28 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index bcf772e..a1bb8b9 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -35,8 +35,17 @@ import nextflow.script.ast.WorkflowNode; import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.MethodNode; -import org.codehaus.groovy.ast.expr.*; -import org.codehaus.groovy.ast.stmt.*; +import org.codehaus.groovy.ast.expr.BinaryExpression; +import org.codehaus.groovy.ast.expr.ClosureExpression; +import org.codehaus.groovy.ast.expr.DeclarationExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.MethodCallExpression; +import org.codehaus.groovy.ast.expr.PropertyExpression; +import org.codehaus.groovy.ast.expr.TernaryExpression; +import org.codehaus.groovy.ast.expr.TupleExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.ast.stmt.ExpressionStatement; +import org.codehaus.groovy.ast.stmt.IfStatement; import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.syntax.Types; @@ -64,7 +73,7 @@ public DataflowVisitor(SourceUnit sourceUnit, ScriptAstCache ast) { this.sourceUnit = sourceUnit; this.ast = ast; - stackPreds.add(new HashSet<>()); + stackPreds.push(new HashSet<>()); } @Override @@ -160,16 +169,15 @@ private void visitWorkflowPublishers(WorkflowNode node, Map result) @Override public void visitIfElse(IfStatement node) { // visit the conditional expression - var preds = visitWithPreds(node.getBooleanExpression()); - var controlDn = addNode("", Node.Type.CONTROL, null, preds); + var controlPreds = visitWithPreds(node.getBooleanExpression()); + var controlDn = current.addNode("", Node.Type.CONTROL, null, controlPreds); // visit the if branch vc.pushScope(); current.pushSubgraph(controlDn); visitWithPreds(node.getIfBlock()); - + current.popSubgraph(); var ifScope = vc.popScope(); - var ifSubgraph = current.popSubgraph(); // visit the else branch Map elseScope; @@ -178,9 +186,8 @@ public void visitIfElse(IfStatement node) { vc.pushScope(); current.pushSubgraph(controlDn); visitWithPreds(node.getElseBlock()); - - elseScope = vc.popScope(); current.popSubgraph(); + elseScope = vc.popScope(); } else { // if there is no else branch, then the set of active symbols @@ -291,6 +298,23 @@ public void visitDeclarationExpression(DeclarationExpression node) { visitAssignment(node, true); } + @Override + public void visitTernaryExpression(TernaryExpression node) { + var controlPreds = visitWithPreds(node.getBooleanExpression()); + var controlDn = current.addNode("", Node.Type.CONTROL, null, controlPreds); + + current.pushSubgraph(controlDn); + var truePreds = visitWithPreds(node.getTrueExpression()); + current.popSubgraph(); + + current.pushSubgraph(controlDn); + var falsePreds = visitWithPreds(node.getFalseExpression()); + current.popSubgraph(); + + currentPreds().addAll(truePreds); + currentPreds().addAll(falsePreds); + } + @Override public void visitClosureExpression(ClosureExpression node) { // skip closures since they can't contain dataflow logic @@ -429,7 +453,7 @@ private Node getSymbol(String name) { return null; if( preds.size() == 1 ) return preds.iterator().next(); - return addNode(name, Node.Type.NAME, null, preds); + return current.addNode(name, Node.Type.NAME, null, preds); } private Set currentPreds() { diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 1f05642..d367276 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -81,7 +81,8 @@ public String render(String name, Graph graph) { } // render nodes - var subgraphEdges = renderSubgraph(graph.peekSubgraph()); + var allSubgraphs = new HashSet(); + renderSubgraph(graph.peekSubgraph(), allSubgraphs); // render outputs if( outputs.size() > 0 ) { @@ -118,8 +119,10 @@ public String render(String name, Graph graph) { append("v%d --> v%d", dnPred.id, dn.id); } - for( var edge : subgraphEdges ) { - append("v%d --> s%d", edge.source(), edge.target()); + // render subgraph edges + for( var subgraph : allSubgraphs ) { + for( var dnPred : subgraph.preds ) + append("v%d --> s%d", dnPred.id, subgraph.id); } decIndent(); @@ -129,11 +132,14 @@ public String render(String name, Graph graph) { } /** - * Render a subgraph and collect all incident edges. + * Render a subgraph and collect all child subgraphs. * * @param subgraph + * @param allSubgraphs */ - private Set renderSubgraph(Subgraph subgraph) { + private void renderSubgraph(Subgraph subgraph, Set allSubgraphs) { + allSubgraphs.add(subgraph); + if( subgraph.id > 0 ) { append("subgraph s%d[\" \"]", subgraph.id); incIndent(); @@ -153,23 +159,14 @@ private Set renderSubgraph(Subgraph subgraph) { append("click v%d href \"%s\" _blank", dn.id, dn.uri.toString()); } - // render subgraphs and collect incident edges - var incidentEdges = new HashSet(); - for( var dnPred : subgraph.preds ) { - incidentEdges.add(new Edge(dnPred.id, subgraph.id)); - } - - for( var s : subgraph.subgraphs ) { - var edges = renderSubgraph(s); - incidentEdges.addAll(edges); - } + // render subgraphs + for( var s : subgraph.subgraphs ) + renderSubgraph(s, allSubgraphs); if( subgraph.id > 0 ) { decIndent(); append("end"); } - - return incidentEdges; } /** diff --git a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy index ad7e0fe..8539ee3 100644 --- a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy +++ b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy @@ -35,7 +35,7 @@ class PreviewDagTest extends Specification { return true } - def 'should render the DAG preview for a workflow' () { + def 'should handle an if-else statement' () { given: def service = getScriptService() def uri = getUri('main.nf') @@ -100,6 +100,69 @@ class PreviewDagTest extends Specification { ) } + def 'should handle a ternary expression' () { + given: + def service = getScriptService() + def uri = getUri('main.nf') + + expect: + checkDagPreview(service, uri, + '''\ + workflow { + echo = params.echo + ? TOUCH(params.echo) + : DEFAULT() + APPEND(echo) + } + + process TOUCH { + input: + val x + + script: + true + } + + process DEFAULT { + true + } + + process APPEND { + input: + val x + + script: + true + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v0["echo"] + end + v1{ } + v5([APPEND]) + click v5 href "$uri" _blank + subgraph s1[" "] + v2([TOUCH]) + click v2 href "$uri" _blank + end + subgraph s2[" "] + v3([DEFAULT]) + click v3 href "$uri" _blank + end + v0 --> v1 + v0 --> v2 + v2 --> v5 + v3 --> v5 + v1 --> s1 + v1 --> s2 + end + """ + ) + } + def 'should handle variable reassignment in an if statement' () { given: def service = getScriptService() From c790455a71c42529a99a5ab1c3176b172c5e5ba0 Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Wed, 30 Jul 2025 14:47:39 -0500 Subject: [PATCH 10/15] fix flaky test, cleanup --- .../services/script/dag/DataflowVisitor.java | 3 +-- .../lsp/services/script/dag/Graph.java | 10 +++++----- .../services/script/dag/MermaidRenderer.java | 19 ++++++++----------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index a1bb8b9..f414f91 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -306,12 +306,11 @@ public void visitTernaryExpression(TernaryExpression node) { current.pushSubgraph(controlDn); var truePreds = visitWithPreds(node.getTrueExpression()); current.popSubgraph(); + currentPreds().addAll(truePreds); current.pushSubgraph(controlDn); var falsePreds = visitWithPreds(node.getFalseExpression()); current.popSubgraph(); - - currentPreds().addAll(truePreds); currentPreds().addAll(falsePreds); } diff --git a/src/main/java/nextflow/lsp/services/script/dag/Graph.java b/src/main/java/nextflow/lsp/services/script/dag/Graph.java index 14e3bfa..98f271c 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/Graph.java +++ b/src/main/java/nextflow/lsp/services/script/dag/Graph.java @@ -51,12 +51,12 @@ public Subgraph peekSubgraph() { } public void pushSubgraph() { - subgraphs.push(new Subgraph(nextSubgraphId, Collections.emptySet())); + subgraphs.push(new Subgraph(nextSubgraphId, null)); nextSubgraphId += 1; } public void pushSubgraph(Node dn) { - subgraphs.push(new Subgraph(nextSubgraphId, Set.of(dn))); + subgraphs.push(new Subgraph(nextSubgraphId, dn)); nextSubgraphId += 1; } @@ -80,15 +80,15 @@ class Subgraph { public final int id; - public final Set preds; + public final Node pred; public final List subgraphs = new ArrayList<>(); public final Set nodes = new HashSet<>(); - public Subgraph(int id, Set preds) { + public Subgraph(int id, Node pred) { this.id = id; - this.preds = preds; + this.pred = pred; } } diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index d367276..5f798f8 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -15,8 +15,10 @@ */ package nextflow.lsp.services.script.dag; +import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -28,7 +30,7 @@ */ public class MermaidRenderer { - private final boolean showVariables = false; + private static final boolean SHOW_VARIABLES = false; private StringBuilder builder; @@ -81,7 +83,7 @@ public String render(String name, Graph graph) { } // render nodes - var allSubgraphs = new HashSet(); + var allSubgraphs = new ArrayList(); renderSubgraph(graph.peekSubgraph(), allSubgraphs); // render outputs @@ -121,8 +123,8 @@ public String render(String name, Graph graph) { // render subgraph edges for( var subgraph : allSubgraphs ) { - for( var dnPred : subgraph.preds ) - append("v%d --> s%d", dnPred.id, subgraph.id); + if( subgraph.pred != null ) + append("v%d --> s%d", subgraph.pred.id, subgraph.id); } decIndent(); @@ -137,7 +139,7 @@ public String render(String name, Graph graph) { * @param subgraph * @param allSubgraphs */ - private void renderSubgraph(Subgraph subgraph, Set allSubgraphs) { + private void renderSubgraph(Subgraph subgraph, List allSubgraphs) { allSubgraphs.add(subgraph); if( subgraph.id > 0 ) { @@ -181,7 +183,7 @@ private boolean isHidden(Node dn, Collection inputs, Collection outp } private boolean isHidden(Node dn) { - return !showVariables && dn.type == Node.Type.NAME; + return !SHOW_VARIABLES && dn.type == Node.Type.NAME; } private static String renderNode(int id, String label, Node.Type type) { @@ -192,9 +194,4 @@ private static String renderNode(int id, String label, Node.Type type) { }; } - private static record Edge( - int source, - int target - ) {} - } From 75f6c7727f1cd0fc44b8a27b942f1b7b265bcbcb Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Thu, 31 Jul 2025 10:43:06 -0500 Subject: [PATCH 11/15] Fix rendering of variable nodes --- .../java/nextflow/lsp/services/script/dag/MermaidRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 5f798f8..e3fd0d8 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -149,7 +149,7 @@ private void renderSubgraph(Subgraph subgraph, List allSubgraphs) { // render nodes for( var dn : subgraph.nodes ) { - if( dn.type == Node.Type.NAME ) + if( isHidden(dn) ) continue; var label = dn.label From 4559d4b3f8903e308c8e958bedb7682c77c914ee Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Thu, 31 Jul 2025 11:30:00 -0500 Subject: [PATCH 12/15] Add configuration settings for direction and verbose --- .../java/nextflow/lsp/NextflowLanguageServer.java | 6 ++++-- .../lsp/services/LanguageServerConfiguration.java | 4 ++++ .../nextflow/lsp/services/LanguageService.java | 2 +- .../services/script/ScriptCodeLensProvider.java | 4 ++-- .../lsp/services/script/ScriptService.java | 4 ++-- .../lsp/services/script/dag/MermaidRenderer.java | 15 ++++++++++++--- .../lsp/services/script/dag/PreviewDagTest.groovy | 3 ++- 7 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/main/java/nextflow/lsp/NextflowLanguageServer.java b/src/main/java/nextflow/lsp/NextflowLanguageServer.java index 84562c1..8b2ed21 100644 --- a/src/main/java/nextflow/lsp/NextflowLanguageServer.java +++ b/src/main/java/nextflow/lsp/NextflowLanguageServer.java @@ -450,6 +450,8 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) { var oldConfiguration = configuration; configuration = new LanguageServerConfiguration( + withDefault(JsonUtils.getString(settings, "nextflow.dag.direction"), configuration.dagDirection()), + withDefault(JsonUtils.getBoolean(settings, "nextflow.dag.verbose"), configuration.dagVerbose()), withDefault(errorReportingMode(settings), configuration.errorReportingMode()), withDefault(JsonUtils.getStringArray(settings, "nextflow.files.exclude"), configuration.excludePatterns()), withDefault(JsonUtils.getBoolean(settings, "nextflow.completion.extended"), configuration.extendedCompletion()), @@ -553,14 +555,14 @@ public CompletableFuture executeCommand(ExecuteCommandParams params) { var uri = JsonUtils.getString(arguments.get(0)); var service = getLanguageService(uri); if( service != null ) - return service.executeCommand(command, arguments); + return service.executeCommand(command, arguments, configuration); } if( "nextflow.server.previewWorkspace".equals(command) && arguments.size() == 1 ) { log.debug(String.format("textDocument/previewWorkspace %s", arguments.toString())); var name = JsonUtils.getString(arguments.get(0)); var service = scriptServices.get(name); if( service != null ) - return service.executeCommand(command, arguments); + return service.executeCommand(command, arguments, configuration); } return null; }); diff --git a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java index 2c603ed..be9ad2c 100644 --- a/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java +++ b/src/main/java/nextflow/lsp/services/LanguageServerConfiguration.java @@ -19,6 +19,8 @@ import java.util.List; public record LanguageServerConfiguration( + String dagDirection, + boolean dagVerbose, ErrorReportingMode errorReportingMode, List excludePatterns, boolean extendedCompletion, @@ -31,6 +33,8 @@ public record LanguageServerConfiguration( public static LanguageServerConfiguration defaults() { return new LanguageServerConfiguration( + "TB", + false, ErrorReportingMode.WARNINGS, Collections.emptyList(), false, diff --git a/src/main/java/nextflow/lsp/services/LanguageService.java b/src/main/java/nextflow/lsp/services/LanguageService.java index bf352e0..3c8ffc9 100644 --- a/src/main/java/nextflow/lsp/services/LanguageService.java +++ b/src/main/java/nextflow/lsp/services/LanguageService.java @@ -230,7 +230,7 @@ public List> documentSymbol(DocumentSy return provider.documentSymbol(params.getTextDocument()); } - public Object executeCommand(String command, List arguments) { + public Object executeCommand(String command, List arguments, LanguageServerConfiguration configuration) { return null; } diff --git a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java index 02745a3..3774f4e 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java @@ -70,7 +70,7 @@ public List codeLens(TextDocumentIdentifier textDocument) { return result; } - public Map previewDag(String documentUri, String name) { + public Map previewDag(String documentUri, String name, String direction, boolean verbose) { var uri = URI.create(documentUri); if( !ast.hasAST(uri) || ast.hasErrors(uri) ) return Map.of("error", "DAG preview cannot be shown because the script has errors."); @@ -84,7 +84,7 @@ public Map previewDag(String documentUri, String name) { visitor.visit(); var graph = visitor.getGraph(wn.isEntry() ? "" : wn.getName()); - var result = new MermaidRenderer().render(wn.getName(), graph); + var result = new MermaidRenderer(direction, verbose).render(wn.getName(), graph); log.debug(result); return Map.of("result", result); }) diff --git a/src/main/java/nextflow/lsp/services/script/ScriptService.java b/src/main/java/nextflow/lsp/services/script/ScriptService.java index a4ead65..bc79c80 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptService.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptService.java @@ -121,13 +121,13 @@ protected SymbolProvider getSymbolProvider() { } @Override - public Object executeCommand(String command, List arguments) { + public Object executeCommand(String command, List arguments, LanguageServerConfiguration configuration) { updateNow(); if( "nextflow.server.previewDag".equals(command) && arguments.size() == 2 ) { var uri = getJsonString(arguments.get(0)); var name = getJsonString(arguments.get(1)); var provider = new ScriptCodeLensProvider(astCache); - return provider.previewDag(uri, name); + return provider.previewDag(uri, name, configuration.dagDirection(), configuration.dagVerbose()); } if( "nextflow.server.previewWorkspace".equals(command) ) { var provider = new WorkspacePreviewProvider(astCache); diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index e3fd0d8..3882624 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -30,12 +30,21 @@ */ public class MermaidRenderer { - private static final boolean SHOW_VARIABLES = false; + private static final List VALID_DIRECTIONS = List.of("LR", "TB", "TD"); + + private final String direction; + + private final boolean verbose; private StringBuilder builder; private int indent; + public MermaidRenderer(String direction, boolean verbose) { + this.direction = VALID_DIRECTIONS.contains(direction) ? direction : "TB"; + this.verbose = verbose; + } + private void reset() { builder = new StringBuilder(); indent = 0; @@ -64,7 +73,7 @@ public String render(String name, Graph graph) { // render graph reset(); - append("flowchart TB"); + append("flowchart %s", direction); incIndent(); append("subgraph %s", isEntry ? "\" \"" : name); incIndent(); @@ -183,7 +192,7 @@ private boolean isHidden(Node dn, Collection inputs, Collection outp } private boolean isHidden(Node dn) { - return !SHOW_VARIABLES && dn.type == Node.Type.NAME; + return !verbose && dn.type == Node.Type.NAME; } private static String renderNode(int id, String label, Node.Type type) { diff --git a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy index 8539ee3..e19ff93 100644 --- a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy +++ b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy @@ -16,6 +16,7 @@ package nextflow.lsp.services.script.dag +import nextflow.lsp.services.LanguageServerConfiguration import nextflow.lsp.services.script.ScriptService import spock.lang.Specification @@ -30,7 +31,7 @@ class PreviewDagTest extends Specification { boolean checkDagPreview(ScriptService service, String uri, String source, String mmd) { open(service, uri, source.stripIndent()) - def response = service.executeCommand('nextflow.server.previewDag', [asJson(uri), asJson(null)]) + def response = service.executeCommand('nextflow.server.previewDag', [asJson(uri), asJson(null)], LanguageServerConfiguration.defaults()) assert response.result == mmd.stripIndent() return true } From 845dcfe90ec17566f4f4ffb92bdf6f9df4c2dff1 Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Thu, 31 Jul 2025 17:55:06 -0500 Subject: [PATCH 13/15] Hide empty and disconnected subgraphs --- .../services/script/dag/DataflowVisitor.java | 76 +++++----- .../lsp/services/script/dag/Graph.java | 20 ++- .../services/script/dag/MermaidRenderer.java | 135 +++++++++++------- .../services/script/dag/VariableContext.java | 72 ++++------ .../services/script/dag/PreviewDagTest.groovy | 105 +++++++++++++- 5 files changed, 279 insertions(+), 129 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index f414f91..cbad4a2 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -46,6 +46,7 @@ import org.codehaus.groovy.ast.expr.VariableExpression; import org.codehaus.groovy.ast.stmt.ExpressionStatement; import org.codehaus.groovy.ast.stmt.IfStatement; +import org.codehaus.groovy.ast.stmt.Statement; import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.syntax.Types; @@ -108,10 +109,8 @@ public void visitWorkflow(WorkflowNode node) { var name = node.isEntry() ? "" : node.getName(); visitWorkflowTakes(node, current.inputs); visit(node.main); - if( node.isEntry() ) - visitWorkflowPublishers(node, current.outputs); - else - visitWorkflowEmits(node, current.outputs); + var outputs = node.isEntry() ? node.publishers : node.emits; + visitWorkflowOutputs(outputs, current.outputs); graphs.put(name, current); inEntry = false; current = null; @@ -121,13 +120,14 @@ private void visitWorkflowTakes(WorkflowNode node, Map result) { for( var stmt : asBlockStatements(node.takes) ) { var name = asVarX(stmt).getName(); var dn = addNode(name, Node.Type.NAME, stmt); + dn.verbose = false; vc.putSymbol(name, dn); result.put(name, dn); } } - private void visitWorkflowEmits(WorkflowNode node, Map result) { - for( var stmt : asBlockStatements(node.emits) ) { + private void visitWorkflowOutputs(Statement outputs, Map result) { + for( var stmt : asBlockStatements(outputs) ) { var emit = ((ExpressionStatement) stmt).getExpression(); String name; if( emit instanceof VariableExpression ve ) { @@ -143,23 +143,10 @@ else if( emit instanceof AssignmentExpression assign ) { visit(new AssignmentExpression(varX(name), emit)); } var dn = getSymbol(name); - if( dn == null ) - System.err.println("missing emit: " + name); - result.put(name, dn); - } - } - - private void visitWorkflowPublishers(WorkflowNode node, Map result) { - for( var stmt : asBlockStatements(node.publishers) ) { - var es = (ExpressionStatement) stmt; - var publisher = (BinaryExpression) es.getExpression(); - var target = asVarX(publisher.getLeftExpression()); - var source = publisher.getRightExpression(); - visit(new AssignmentExpression(target, source)); - var name = target.getName(); - var dn = getSymbol(name); - if( dn == null ) - System.err.println("missing publisher: " + name); + if( dn != null ) + dn.verbose = false; + else + System.err.println("missing output: " + name); result.put(name, dn); } } @@ -176,27 +163,45 @@ public void visitIfElse(IfStatement node) { vc.pushScope(); current.pushSubgraph(controlDn); visitWithPreds(node.getIfBlock()); - current.popSubgraph(); + var ifSubgraph = current.popSubgraph(); var ifScope = vc.popScope(); // visit the else branch + Subgraph elseSubgraph; Map elseScope; if( !node.getElseBlock().isEmpty() ) { vc.pushScope(); current.pushSubgraph(controlDn); visitWithPreds(node.getElseBlock()); - current.popSubgraph(); + elseSubgraph = current.popSubgraph(); elseScope = vc.popScope(); } else { // if there is no else branch, then the set of active symbols // after the if statement is the union of the active symbols // from before the if and the active symbols in the if + elseSubgraph = null; elseScope = vc.peekScope(); } - vc.mergeConditionalScopes(ifScope, elseScope); + // apply variables from if and else scopes to current scope + var outputs = vc.mergeConditionalScopes(ifScope, elseScope); + + for( var name : outputs ) { + var preds = vc.getSymbolPreds(name); + if( preds.size() > 1 ) { + var dn = current.addNode(name, Node.Type.NAME, null, preds); + vc.putSymbol(name, dn); + } + } + + // hide if-else statement if both subgraphs are empty + if( ifSubgraph.isVerbose() && (elseSubgraph == null || elseSubgraph.isVerbose()) ) { + controlDn.verbose = true; + for( var name : outputs ) + getSymbol(name).preds.addAll(controlPreds); + } } // expressions @@ -305,13 +310,19 @@ public void visitTernaryExpression(TernaryExpression node) { current.pushSubgraph(controlDn); var truePreds = visitWithPreds(node.getTrueExpression()); - current.popSubgraph(); + var trueSubgraph = current.popSubgraph(); currentPreds().addAll(truePreds); current.pushSubgraph(controlDn); var falsePreds = visitWithPreds(node.getFalseExpression()); - current.popSubgraph(); + var falseSubgraph = current.popSubgraph(); currentPreds().addAll(falsePreds); + + // hide ternary expression if both subgraphs are empty + if( trueSubgraph.isVerbose() && falseSubgraph.isVerbose() ) { + controlDn.verbose = true; + currentPreds().addAll(controlPreds); + } } @Override @@ -326,6 +337,7 @@ public void visitPropertyExpression(PropertyExpression node) { if( !current.inputs.containsKey(name) ) current.inputs.put(name, addNode(name, Node.Type.NAME, null)); var dn = current.inputs.get(name); + dn.verbose = false; currentPreds().add(dn); return; } @@ -447,12 +459,12 @@ public void visitVariableExpression(VariableExpression node) { // helpers private Node getSymbol(String name) { - var preds = vc.getSymbol(name); + var preds = vc.getSymbolPreds(name); if( preds.isEmpty() ) return null; - if( preds.size() == 1 ) - return preds.iterator().next(); - return current.addNode(name, Node.Type.NAME, null, preds); + if( preds.size() > 1 ) + System.err.println("unmerged symbol " + name + " " + preds); + return preds.iterator().next(); } private Set currentPreds() { diff --git a/src/main/java/nextflow/lsp/services/script/dag/Graph.java b/src/main/java/nextflow/lsp/services/script/dag/Graph.java index 98f271c..f913281 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/Graph.java +++ b/src/main/java/nextflow/lsp/services/script/dag/Graph.java @@ -90,6 +90,20 @@ public Subgraph(int id, Node pred) { this.id = id; this.pred = pred; } + + public boolean isVerbose() { + return nodes.stream().allMatch(n -> n.verbose); + } + + @Override + public boolean equals(Object other) { + return other instanceof Subgraph s && this.id == s.id; + } + + @Override + public int hashCode() { + return id; + } } @@ -106,12 +120,15 @@ public enum Type { public final URI uri; public final Set preds; + public boolean verbose; + public Node(int id, String label, Type type, URI uri, Set preds) { this.id = id; this.label = label; this.type = type; this.uri = uri; this.preds = preds; + this.verbose = (type == Type.NAME); } public void addPredecessors(Set preds) { @@ -130,6 +147,7 @@ public int hashCode() { @Override public String toString() { - return String.format("id=%s,label='%s',type=%s", id, label, type); + var predIds = preds.stream().map(p -> p.id).toList(); + return String.format("id=%s,label='%s',type=%s,preds=%s", id, label, type, predIds); } } diff --git a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java index 3882624..5d2d2f9 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java +++ b/src/main/java/nextflow/lsp/services/script/dag/MermaidRenderer.java @@ -17,8 +17,10 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -91,9 +93,17 @@ public String render(String name, Graph graph) { append("end"); } - // render nodes + // render nodes and subgraphs + var root = graph.peekSubgraph(); + + root.nodes.stream() + .filter(n -> !inputs.contains(n)) + .filter(n -> !outputs.contains(n)) + .forEach(this::renderNode); + var allSubgraphs = new ArrayList(); - renderSubgraph(graph.peekSubgraph(), allSubgraphs); + for( var s : root.subgraphs ) + renderSubgraph(s, allSubgraphs); // render outputs if( outputs.size() > 0 ) { @@ -106,27 +116,13 @@ public String render(String name, Graph graph) { } // render edges + var visited = new HashMap>(); + for( var dn : nodes ) { - if( isHidden(dn, inputs, outputs) ) + if( isHidden(dn) ) continue; - var preds = dn.preds; - var visited = new HashSet(); - while( true ) { - var done = preds.stream().allMatch(p -> !isHidden(p, inputs, outputs)); - if( done ) - break; - visited.addAll(preds); - preds = preds.stream() - .flatMap(pred -> ( - isHidden(pred, inputs, outputs) - ? pred.preds.stream().filter(p -> !visited.contains(p)) - : Stream.of(pred) - )) - .collect(Collectors.toSet()); - } - - for( var dnPred : preds ) + for( var dnPred : visiblePreds(dn, visited) ) append("v%d --> v%d", dnPred.id, dn.id); } @@ -149,50 +145,40 @@ public String render(String name, Graph graph) { * @param allSubgraphs */ private void renderSubgraph(Subgraph subgraph, List allSubgraphs) { - allSubgraphs.add(subgraph); - - if( subgraph.id > 0 ) { - append("subgraph s%d[\" \"]", subgraph.id); - incIndent(); - } + if( isHidden(subgraph) ) + return; - // render nodes - for( var dn : subgraph.nodes ) { - if( isHidden(dn) ) - continue; + allSubgraphs.add(subgraph); - var label = dn.label - .replaceAll("\n", "\\\\\n") - .replaceAll("\"", "\\\\\""); + append("subgraph s%d[\" \"]", subgraph.id); + incIndent(); - append(renderNode(dn.id, label, dn.type)); - if( dn.uri != null ) - append("click v%d href \"%s\" _blank", dn.id, dn.uri.toString()); - } + for( var dn : subgraph.nodes ) + renderNode(dn); - // render subgraphs for( var s : subgraph.subgraphs ) renderSubgraph(s, allSubgraphs); - if( subgraph.id > 0 ) { - decIndent(); - append("end"); - } + decIndent(); + append("end"); } /** - * Only inputs, outputs, and processes/workflows are currently shown. + * Render a node. * * @param dn - * @param inputs - * @param outputs */ - private boolean isHidden(Node dn, Collection inputs, Collection outputs) { - return isHidden(dn) && !inputs.contains(dn) && !outputs.contains(dn); - } + private void renderNode(Node dn) { + if( isHidden(dn) ) + return; - private boolean isHidden(Node dn) { - return !verbose && dn.type == Node.Type.NAME; + var label = dn.label + .replaceAll("\n", "\\\\\n") + .replaceAll("\"", "\\\\\""); + + append(renderNode(dn.id, label, dn.type)); + if( dn.uri != null ) + append("click v%d href \"%s\" _blank", dn.id, dn.uri.toString()); } private static String renderNode(int id, String label, Node.Type type) { @@ -203,4 +189,53 @@ private static String renderNode(int id, String label, Node.Type type) { }; } + /** + * Get the set of visible predecessors for a node. + * + * @param dn + * @param visited + */ + private Set visiblePreds(Node dn, Map> visited) { + if( visited.containsKey(dn) ) + return visited.get(dn); + + var result = dn.preds.stream() + .flatMap(pred -> ( + isHidden(pred) + ? visiblePreds(pred, visited).stream() + : Stream.of(pred) + )) + .collect(Collectors.toSet()); + visited.put(dn, result); + return result; + } + + /** + * When verbose mode is disabled, all nodes marked as verbose are hidden. + * Otherwise, only control nodes marked as verbose are hidden (because they + * are disconnected). + * + * @param dn + */ + private boolean isHidden(Node dn) { + if( verbose ) + return dn.verbose && dn.type == Node.Type.CONTROL; + else + return dn.verbose; + } + + /** + * When verbose mode is disabled, subgraphs with no visible nodes + * are hidden. Otherwise, only subgraphs with no nodes are hidden + * (because they are disconnected). + * + * @param dn + */ + private boolean isHidden(Subgraph s) { + if( verbose ) + return s.nodes.isEmpty(); + else + return s.isVerbose(); + } + } diff --git a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java index 01b0270..9db67ac 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java +++ b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java @@ -48,9 +48,6 @@ public Map peekScope() { */ public void pushScope() { var newScope = new HashMap(); - scopes.peek().forEach((name, variable) -> { - newScope.put(name, variable.shallowCopy()); - }); scopes.push(newScope); } @@ -62,14 +59,27 @@ public Map popScope() { } /** - * Get the current predecessors for a given symbol. + * Get the active instance of a given symbol. * * @param name */ - public Set getSymbol(String name) { - var scope = scopes.peek(); - return scope.containsKey(name) - ? scope.get(name).preds + public Variable getSymbol(String name) { + for( var scope : scopes ) { + if( scope.containsKey(name) ) + return scope.get(name); + } + return null; + } + + /** + * Get the current predecessors of a given symbol. + * + * @param name + */ + public Set getSymbolPreds(String name) { + var variable = getSymbol(name); + return variable != null + ? variable.preds : Collections.emptySet(); } @@ -81,20 +91,12 @@ public Set getSymbol(String name) { * @param isLocal */ public void putSymbol(String name, Node dn, boolean isLocal) { - var scope = scopes.peek(); - if( scope.containsKey(name) ) { - // reassign variable if it is already defined - var variable = scope.get(name); - variable.preds.clear(); - variable.preds.add(dn); - } - else { - var depth = isLocal ? currentDepth() : 1; - var preds = new HashSet(); - preds.add(dn); - var variable = new Variable(depth, preds); - scope.put(name, variable); - } + var variable = getSymbol(name); + var depth = variable != null + ? variable.depth + : isLocal ? currentDepth() : 1; + var preds = Set.of(dn); + scopes.peek().put(name, new Variable(depth, preds)); } public void putSymbol(String name, Node dn) { @@ -107,35 +109,27 @@ public void putSymbol(String name, Node dn) { * @param ifScope * @param elseScope */ - public void mergeConditionalScopes(Map ifScope, Map elseScope) { + public Set mergeConditionalScopes(Map ifScope, Map elseScope) { var allSymbols = new HashMap(); - // add symbols from if branch + // add symbols from if scope ifScope.forEach((name, variable) -> { if( variable.depth > currentDepth() ) return; + // merge symbols from else scope where applicable var other = elseScope.get(name); if( other != null ) variable = variable.union(other); - if( allSymbols.containsKey(name) ) - allSymbols.put(name, allSymbols.get(name).union(variable)); - else - allSymbols.put(name, variable.shallowCopy()); - }); - - // add remaining symbols from else branch - elseScope.forEach((name, variable) -> { - if( variable.depth > currentDepth() ) - return; - - if( !allSymbols.containsKey(name) ) - allSymbols.put(name, variable.shallowCopy()); + allSymbols.put(name, variable); }); // add merged symbols to current scope scopes.peek().putAll(allSymbols); + + // return the set of merged symbols + return allSymbols.keySet(); } private int currentDepth() { @@ -156,10 +150,6 @@ class Variable { this.preds = preds; } - public Variable shallowCopy() { - return new Variable(depth, new HashSet(preds)); - } - public Variable union(Variable other) { var allPreds = new HashSet(preds); allPreds.addAll(other.preds); diff --git a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy index e19ff93..aad1581 100644 --- a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy +++ b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy @@ -29,9 +29,9 @@ import static nextflow.lsp.util.JsonUtils.* */ class PreviewDagTest extends Specification { - boolean checkDagPreview(ScriptService service, String uri, String source, String mmd) { + boolean checkDagPreview(ScriptService service, String uri, String name, String source, String mmd) { open(service, uri, source.stripIndent()) - def response = service.executeCommand('nextflow.server.previewDag', [asJson(uri), asJson(null)], LanguageServerConfiguration.defaults()) + def response = service.executeCommand('nextflow.server.previewDag', [asJson(uri), asJson(name)], LanguageServerConfiguration.defaults()) assert response.result == mmd.stripIndent() return true } @@ -42,7 +42,7 @@ class PreviewDagTest extends Specification { def uri = getUri('main.nf') expect: - checkDagPreview(service, uri, + checkDagPreview(service, uri, null, '''\ workflow { if (params.echo) { @@ -101,13 +101,77 @@ class PreviewDagTest extends Specification { ) } + def 'should collapse empty if-else statement' () { + given: + def service = getScriptService() + def uri = getUri('main.nf') + + expect: + checkDagPreview(service, uri, null, + '''\ + workflow { + main: + if( params.echo ) + echo = 1 + else + echo = 0 + + publish: + echo = echo + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v0["echo"] + end + subgraph publish + v5["echo"] + end + v0 --> v5 + end + """ + ) + + checkDagPreview(service, uri, 'ECHO', + '''\ + workflow ECHO { + take: + debug + + main: + if( debug ) + echo = 1 + else + echo = 0 + + emit: + echo = echo + } + ''', + """\ + flowchart TB + subgraph ECHO + subgraph take + v0["debug"] + end + subgraph emit + v5["echo"] + end + v0 --> v5 + end + """ + ) + } + def 'should handle a ternary expression' () { given: def service = getScriptService() def uri = getUri('main.nf') expect: - checkDagPreview(service, uri, + checkDagPreview(service, uri, null, '''\ workflow { echo = params.echo @@ -164,13 +228,44 @@ class PreviewDagTest extends Specification { ) } + def 'should collapse empty ternary expression' () { + given: + def service = getScriptService() + def uri = getUri('main.nf') + + expect: + checkDagPreview(service, uri, null, + '''\ + workflow { + main: + echo = params.echo ? 1 : 0 + + publish: + echo = echo + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v0["echo"] + end + subgraph publish + v3["echo"] + end + v0 --> v3 + end + """ + ) + } + def 'should handle variable reassignment in an if statement' () { given: def service = getScriptService() def uri = getUri('main.nf') expect: - checkDagPreview(service, uri, + checkDagPreview(service, uri, null, '''\ workflow { main: From 8a16f9da0a747008f48ce0e5190aac6c6cb439bc Mon Sep 17 00:00:00 2001 From: ErikDanielsson Date: Fri, 1 Aug 2025 16:07:07 +0200 Subject: [PATCH 14/15] Add back checking else scope for global variables --- .../lsp/services/script/dag/VariableContext.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java index 9db67ac..04424c1 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java +++ b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java @@ -122,9 +122,21 @@ public Set mergeConditionalScopes(Map ifScope, Map { + if( variable.depth > currentDepth() || ifScope.containsKey(name) ) + return; // The variable is local or declared in the if branch + + allSymbols.put(name, variable); // NOTE (Erik 2025-08-01): replace this line an appropriate compilation error + }); + // add merged symbols to current scope scopes.peek().putAll(allSymbols); From f4a715e023a158ade683ade632d3661529033b0d Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Tue, 5 Aug 2025 18:25:46 -0500 Subject: [PATCH 15/15] Don't collapse if statements in verbose mode --- .../script/ScriptCodeLensProvider.java | 2 +- .../services/script/dag/DataflowVisitor.java | 7 ++- .../services/script/dag/VariableContext.java | 18 ++----- .../services/script/dag/PreviewDagTest.groovy | 49 +++++++++++++++++++ 4 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java index 3774f4e..cf9c5b2 100644 --- a/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java +++ b/src/main/java/nextflow/lsp/services/script/ScriptCodeLensProvider.java @@ -80,7 +80,7 @@ public Map previewDag(String documentUri, String name, String dir .filter(wn -> wn.isEntry() ? name == null : wn.getName().equals(name)) .findFirst() .map((wn) -> { - var visitor = new DataflowVisitor(sourceUnit, ast); + var visitor = new DataflowVisitor(sourceUnit, ast, verbose); visitor.visit(); var graph = visitor.getGraph(wn.isEntry() ? "" : wn.getName()); diff --git a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java index cbad4a2..b971d84 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java +++ b/src/main/java/nextflow/lsp/services/script/dag/DataflowVisitor.java @@ -64,15 +64,18 @@ public class DataflowVisitor extends ScriptVisitorSupport { private ScriptAstCache ast; + private boolean verbose; + private Map graphs = new HashMap<>(); private Stack> stackPreds = new Stack<>(); private VariableContext vc = new VariableContext(); - public DataflowVisitor(SourceUnit sourceUnit, ScriptAstCache ast) { + public DataflowVisitor(SourceUnit sourceUnit, ScriptAstCache ast, boolean verbose) { this.sourceUnit = sourceUnit; this.ast = ast; + this.verbose = verbose; stackPreds.push(new HashSet<>()); } @@ -197,7 +200,7 @@ public void visitIfElse(IfStatement node) { } // hide if-else statement if both subgraphs are empty - if( ifSubgraph.isVerbose() && (elseSubgraph == null || elseSubgraph.isVerbose()) ) { + if( !verbose && ifSubgraph.isVerbose() && (elseSubgraph == null || elseSubgraph.isVerbose()) ) { controlDn.verbose = true; for( var name : outputs ) getSymbol(name).preds.addAll(controlPreds); diff --git a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java index 04424c1..0d6ca9b 100644 --- a/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java +++ b/src/main/java/nextflow/lsp/services/script/dag/VariableContext.java @@ -117,25 +117,15 @@ public Set mergeConditionalScopes(Map ifScope, Map currentDepth() ) return; - // merge symbols from else scope where applicable + // propagate symbols that are definitely assigned var other = elseScope.get(name); if( other != null ) - variable = variable.union(other); + allSymbols.put(name, variable.union(other)); - // NOTE (Erik 2025-08-01): if `other == null` then we should issue a compilation error - // since the variable is not declared in the else branch or preceding outside scope - - allSymbols.put(name, variable); + // TODO: variables assigned in if but not else (or outside scope) are not definitely assigned }); - // Add variables defined in the else branch but not in the if branch - // It is here we check that all variables declared in the else branch are declared in the if branch - elseScope.forEach((name, variable) -> { - if( variable.depth > currentDepth() || ifScope.containsKey(name) ) - return; // The variable is local or declared in the if branch - - allSymbols.put(name, variable); // NOTE (Erik 2025-08-01): replace this line an appropriate compilation error - }); + // TODO: variables assigned in else but not if are not definitely assigned // add merged symbols to current scope scopes.peek().putAll(allSymbols); diff --git a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy index aad1581..4ed81b1 100644 --- a/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy +++ b/src/test/groovy/nextflow/lsp/services/script/dag/PreviewDagTest.groovy @@ -99,6 +99,55 @@ class PreviewDagTest extends Specification { end """ ) + + checkDagPreview(service, uri, null, + '''\ + workflow { + main: + if (TEST(params.input)) { + intermed = 1 + } else { + intermed = 2 + } + publish: + result = APPLY(intermed) + } + + process TEST { + input: + val x + + script: + true + } + + process APPLY { + input: + val x + + script: + true + } + ''', + """\ + flowchart TB + subgraph " " + subgraph params + v0["input"] + end + v1([TEST]) + click v1 href "$uri" _blank + v6([APPLY]) + click v6 href "$uri" _blank + subgraph publish + v7["result"] + end + v0 --> v1 + v1 --> v6 + v6 --> v7 + end + """ + ) } def 'should collapse empty if-else statement' () {