Skip to content

Commit 99f9edf

Browse files
bentshermanbentsherman
authored
Fix explicit it closure parameter (#6276)
Signed-off-by: Ben Sherman <[email protected]>
1 parent 2d76d8f commit 99f9edf

File tree

8 files changed

+88
-58
lines changed

8 files changed

+88
-58
lines changed

‎modules/nextflow/src/test/groovy/nextflow/script/parser/v2/ScriptLoaderV2Test.groovy‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,4 +119,24 @@ class ScriptLoaderV2Test extends Specification {
119119
meta.getWorkflow('hello').declaredOutputs == ['result']
120120
}
121121

122+
def 'should allow explicit `it` closure parameter' () {
123+
124+
given:
125+
def session = new Session()
126+
def parser = new ScriptLoaderV2(session)
127+
128+
def TEXT = '''
129+
workflow {
130+
channel.of(1, 2, 3).view { it -> "${it}" }
131+
}
132+
'''
133+
134+
when:
135+
parser.parse(TEXT)
136+
parser.runScript()
137+
138+
then:
139+
noExceptionThrown()
140+
}
141+
122142
}

‎modules/nf-lang/src/main/java/nextflow/config/control/ConfigToGroovyVisitor.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ protected Statement transformConfigApplyBlock(ConfigApplyBlockNode node) {
7474
for( var call : node.statements )
7575
statements.add(stmt(call));
7676
var code = block(new VariableScope(), statements);
77-
return stmt(callThisX("block", args(constX(node.name), closureX(code))));
77+
return stmt(callThisX("block", args(constX(node.name), closureX(null, code))));
7878
}
7979

8080
@Override
@@ -108,7 +108,7 @@ else if( stmt instanceof ConfigIncludeNode cin )
108108
}
109109
var code = block(new VariableScope(), statements);
110110
var kind = node.kind != null ? node.kind : "block";
111-
return stmt(callThisX(kind, args(constX(node.name), closureX(code))));
111+
return stmt(callThisX(kind, args(constX(node.name), closureX(null, code))));
112112
}
113113

114114
@Override

‎modules/nf-lang/src/main/java/nextflow/config/control/VariableScopeVisitor.java‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import nextflow.config.dsl.ConfigDsl;
3030
import nextflow.config.schema.SchemaNode;
3131
import nextflow.script.ast.ASTNodeMarker;
32+
import nextflow.script.ast.ImplicitClosureParameter;
3233
import nextflow.script.control.VariableScopeChecker;
3334
import nextflow.script.dsl.ProcessDsl;
3435
import nextflow.script.dsl.ScriptDsl;
@@ -288,18 +289,18 @@ public void visitDeclarationExpression(DeclarationExpression node) {
288289
public void visitClosureExpression(ClosureExpression node) {
289290
vsc.pushScope();
290291
node.setVariableScope(currentScope());
291-
if( node.getParameters() != null ) {
292+
if( node.isParameterSpecified() ) {
292293
for( var parameter : node.getParameters() ) {
293294
vsc.declare(parameter, parameter);
294295
if( parameter.hasInitialExpression() )
295-
visit(parameter.getInitialExpression());
296+
parameter.getInitialExpression().visit(this);
296297
}
297298
}
298-
super.visitClosureExpression(node);
299-
for( var it = currentScope().getReferencedLocalVariablesIterator(); it.hasNext(); ) {
300-
var variable = it.next();
301-
variable.setClosureSharedVariable(true);
299+
else if( node.getParameters() != null ) {
300+
var implicit = new ImplicitClosureParameter();
301+
currentScope().putDeclaredVariable(implicit);
302302
}
303+
super.visitClosureExpression(node);
303304
vsc.popScope();
304305
}
305306

@@ -308,16 +309,16 @@ public void visitVariableExpression(VariableExpression node) {
308309
var name = node.getName();
309310
Variable variable = vsc.findVariableDeclaration(name, node);
310311
if( variable == null ) {
311-
if( "it".equals(name) ) {
312-
vsc.addParanoidWarning("Implicit closure parameter `it` will not be supported in a future version", node);
313-
}
314-
else if( inProcess && inClosure ) {
312+
if( inProcess && inClosure ) {
315313
// dynamic process directives can reference process inputs which are not known at this point
316314
}
317315
else {
318316
variable = new DynamicVariable(name, false);
319317
}
320318
}
319+
if( variable instanceof ImplicitClosureParameter ) {
320+
vsc.addWarning("Implicit closure parameter is deprecated, declare an explicit parameter instead", variable.getName(), node);
321+
}
321322
if( variable != null ) {
322323
node.setAccessedVariable(variable);
323324
}

‎modules/nf-lang/src/main/java/nextflow/config/parser/ConfigAstBuilder.java‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,24 +1173,12 @@ private void checkDuplicateNamedArg(List<MapEntryExpression> namedArgs, MapEntry
11731173
/// MISCELLANEOUS
11741174

11751175
private Parameter[] formalParameterList(FormalParameterListContext ctx) {
1176-
// NOTE: implicit `it` parameter is deprecated, but allow it for now
11771176
if( ctx == null )
11781177
return Parameter.EMPTY_ARRAY;
11791178

1180-
var params = ctx.formalParameter().stream()
1179+
return ctx.formalParameter().stream()
11811180
.map(this::formalParameter)
1182-
.toList();
1183-
for( int n = params.size(), i = n - 1; i >= 0; i -= 1 ) {
1184-
var param = params.get(i);
1185-
for( var other : params ) {
1186-
if( other == param )
1187-
continue;
1188-
if( other.getName().equals(param.getName()) )
1189-
throw createParsingFailedException("Duplicated parameter '" + param.getName() + "' found", param);
1190-
}
1191-
}
1192-
1193-
return params.toArray(Parameter.EMPTY_ARRAY);
1181+
.toArray(Parameter[]::new);
11941182
}
11951183

11961184
private Parameter formalParameter(FormalParameterContext ctx) {

‎modules/nf-lang/src/main/java/nextflow/script/ast/ImplicitClosureParameter.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright 2024-2025, Seqera Labs
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package nextflow.script.ast;
17+
18+
import org.codehaus.groovy.ast.ClassHelper;
19+
import org.codehaus.groovy.ast.Parameter;
20+
21+
/**
22+
* An implicit closure parameter (`it`). Used to discourage
23+
* the use of implicit parameters.
24+
*
25+
* @author Ben Sherman <[email protected]>
26+
*/
27+
public class ImplicitClosureParameter extends Parameter {
28+
29+
public ImplicitClosureParameter() {
30+
super(ClassHelper.dynamicType(), "it");
31+
}
32+
}

‎modules/nf-lang/src/main/java/nextflow/script/control/ScriptToGroovyVisitor.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,12 @@ public void visitWorkflow(WorkflowNode node) {
132132
var bodyDef = stmt(createX(
133133
"nextflow.script.BodyDef",
134134
args(
135-
closureX(node.main),
135+
closureX(null, node.main),
136136
constX(getSourceText(node)),
137137
constX("workflow")
138138
)
139139
));
140-
var closure = closureX(block(new VariableScope(), List.of(
140+
var closure = closureX(null, block(new VariableScope(), List.of(
141141
node.takes,
142142
node.emits,
143143
bodyDef
@@ -207,13 +207,13 @@ public void visitProcess(ProcessNode node) {
207207
var bodyDef = stmt(createX(
208208
"nextflow.script.BodyDef",
209209
args(
210-
closureX(node.exec),
210+
closureX(null, node.exec),
211211
constX(getSourceText(node.exec)),
212212
constX(node.type)
213213
)
214214
));
215215
var stub = processStub(node.stub);
216-
var closure = closureX(block(new VariableScope(), List.of(
216+
var closure = closureX(null, block(new VariableScope(), List.of(
217217
node.directives,
218218
node.inputs,
219219
node.outputs,
@@ -350,7 +350,7 @@ private Expression varToStrX(Expression node) {
350350
}
351351

352352
protected ClosureExpression wrapExpressionInClosure(Expression node) {
353-
return closureX(block(stmt(node)));
353+
return closureX(null, block(stmt(node)));
354354
}
355355

356356
private Statement processWhen(Expression when) {
@@ -371,7 +371,7 @@ private Statement processStub(Statement stub) {
371371
return stmt(callThisX("stub", createX(
372372
"nextflow.script.TaskClosure",
373373
args(
374-
closureX(stub),
374+
closureX(null, stub),
375375
constX(getSourceText(stub))
376376
)
377377
)));
@@ -392,11 +392,11 @@ public void visitOutputs(OutputBlockNode node) {
392392
.map((output) -> {
393393
new PublishDslVisitor().visit(output.body);
394394
var name = constX(output.name);
395-
var body = closureX(output.body);
395+
var body = closureX(null, output.body);
396396
return stmt(callThisX("declare", args(name, body)));
397397
})
398398
.toList();
399-
var closure = closureX(block(new VariableScope(), statements));
399+
var closure = closureX(null, block(new VariableScope(), statements));
400400
var result = stmt(callThisX("output", args(closure)));
401401
moduleNode.addStatement(result);
402402
}

‎modules/nf-lang/src/main/java/nextflow/script/control/VariableScopeVisitor.java‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import nextflow.script.ast.AssignmentExpression;
2323
import nextflow.script.ast.FeatureFlagNode;
2424
import nextflow.script.ast.FunctionNode;
25+
import nextflow.script.ast.ImplicitClosureParameter;
2526
import nextflow.script.ast.IncludeNode;
2627
import nextflow.script.ast.OutputNode;
2728
import nextflow.script.ast.ProcessNode;
@@ -621,18 +622,18 @@ public void visitClosureExpression(ClosureExpression node) {
621622

622623
vsc.pushScope();
623624
node.setVariableScope(currentScope());
624-
if( node.getParameters() != null ) {
625+
if( node.isParameterSpecified() ) {
625626
for( var parameter : node.getParameters() ) {
626627
vsc.declare(parameter, parameter);
627628
if( parameter.hasInitialExpression() )
628-
visit(parameter.getInitialExpression());
629+
parameter.getInitialExpression().visit(this);
629630
}
630631
}
631-
super.visitClosureExpression(node);
632-
for( var it = currentScope().getReferencedLocalVariablesIterator(); it.hasNext(); ) {
633-
var variable = it.next();
634-
variable.setClosureSharedVariable(true);
632+
else if( node.getParameters() != null ) {
633+
var implicit = new ImplicitClosureParameter();
634+
currentScope().putDeclaredVariable(implicit);
635635
}
636+
super.visitClosureExpression(node);
636637
vsc.popScope();
637638

638639
currentClosure = cl;
@@ -643,10 +644,7 @@ public void visitVariableExpression(VariableExpression node) {
643644
var name = node.getName();
644645
Variable variable = vsc.findVariableDeclaration(name, node);
645646
if( variable == null ) {
646-
if( "it".equals(name) ) {
647-
vsc.addParanoidWarning("Implicit closure parameter `it` will not be supported in a future version", node);
648-
}
649-
else if( "args".equals(name) ) {
647+
if( "args".equals(name) ) {
650648
vsc.addParanoidWarning("The use of `args` outside the entry workflow will not be supported in a future version", node);
651649
}
652650
else if( "params".equals(name) ) {
@@ -659,6 +657,9 @@ else if( isStdinStdout(name) ) {
659657
variable = new DynamicVariable(name, false);
660658
}
661659
}
660+
if( variable instanceof ImplicitClosureParameter ) {
661+
vsc.addWarning("Implicit closure parameter is deprecated, declare an explicit parameter instead", variable.getName(), node);
662+
}
662663
if( variable != null ) {
663664
checkGlobalVariableInProcess(variable, node);
664665
node.setAccessedVariable(variable);

‎modules/nf-lang/src/main/java/nextflow/script/parser/ScriptAstBuilder.java‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1545,24 +1545,12 @@ private void checkDuplicateNamedArg(List<MapEntryExpression> namedArgs, MapEntry
15451545
/// MISCELLANEOUS
15461546

15471547
private Parameter[] formalParameterList(FormalParameterListContext ctx) {
1548-
// NOTE: implicit `it` parameter is deprecated, but allow it for now
15491548
if( ctx == null )
15501549
return Parameter.EMPTY_ARRAY;
15511550

1552-
var params = ctx.formalParameter().stream()
1551+
return ctx.formalParameter().stream()
15531552
.map(this::formalParameter)
1554-
.toList();
1555-
for( int n = params.size(), i = n - 1; i >= 0; i -= 1 ) {
1556-
var param = params.get(i);
1557-
for( var other : params ) {
1558-
if( other == param )
1559-
continue;
1560-
if( other.getName().equals(param.getName()) )
1561-
throw createParsingFailedException("Duplicated parameter '" + param.getName() + "' found", param);
1562-
}
1563-
}
1564-
1565-
return params.toArray(Parameter.EMPTY_ARRAY);
1553+
.toArray(Parameter[]::new);
15661554
}
15671555

15681556
private Parameter formalParameter(FormalParameterContext ctx) {

0 commit comments

Comments
 (0)