Fix params block breaking piped operators in the entry workflow - #7405
Fix params block breaking piped operators in the entry workflow#7405bentsherman wants to merge 1 commit into
params block breaking piped operators in the entry workflow#7405Conversation
) `VariableScopeVisitor.visitWorkflow` builds the entry workflow class scope with `workflowDsl()`, which copies the legacy `@Operator` methods from `WorkflowDslV1` when static typing is disabled. When the script declared a `params` block, the following branch replaced that node with a fresh `ClassNode` of the same type class in order to override the `getParams()` return type, discarding the operator methods along with it. Any operator invoked via the pipe form in the entry workflow then failed to resolve, e.g. "`view` is not defined". The copy is unnecessary: `workflowDsl()` already returns a fresh `ClassNode` rather than a cached one, so its `getParams()` method can be mutated directly. The same pattern in `visitOutputs()` is correct and is left alone, since that scope really does start from a cached node. Signed-off-by: Ben Sherman <bentshermann@gmail.com>
✅ Deploy Preview for nextflow-docs canceled.
|
pditommaso
left a comment
pditommaso
left a comment
There was a problem hiding this comment.
Verified locally: without the fix the new test fails with `view` is not defined, with it the full :nf-lang:test suite is green. The root cause diagnosis looks right — workflowDsl() already returns a fresh node, so the removed copy was silently dropping the operator methods it had just added.
Two follow-up suggestions and a couple of minor notes.
The remaining asymmetry is a trap
visitOutputs (VariableScopeVisitor.java:506-513) has a near-identical block, but it starts from ClassHelper.makeCached(OutputDsl.class) — a shared cached node — so there the defensive new ClassNode(...) copy is load-bearing and must not be removed. After this PR the two blocks look almost the same while having opposite requirements, and neither states why. That's the same entropy that produced this bug.
Suggestion: remove the convention rather than document it — e.g. let workflowDsl() accept the paramsType and apply the override itself, so "must be a private copy" is enforced by construction. Failing that, a one-line comment at each site.
The test pins only half of the interaction
The new test pins the operator half, but nothing pins the params type override that was clobbering it: TypeCheckingTest.groovy has no params references, and the only other params {} blocks in nf-lang tests are in ScriptAstBuilderTest (parser-level, no scope resolution). If this change had broken the paramsType override instead of fixing operators, the suite would still be green.
Cheap to close in the same test:
- declared param resolves — add
println(params.greeting)to the workflow body - undeclared one still errors —
params.missing, expecting 1 error
Also uncovered: params {} with typing enabled, where operators must not be added but params must still resolve.
Minor
getDeclaredMethods("getParams").get(0)is unguarded at both sites. Pre-existing and fine in practice, butgetDeclaredMethodsignores supertypes, so it would throwIndexOutOfBoundsExceptionifgetParamsever moved to a parent interface.- Unverified and out of scope here, but in the same method:
ClassNode.addMethodinworkflowDsl()may callsetDeclaringClasson theMethodNodes taken from the cachedWorkflowDslV1node, which would mutate shared AST across compilations. Could matter for the long-running language server — worth a separate look.
2 participants
Fix #7399
This PR fixes a bug in the strict parser where a
params {}block causes piped operators likech | viewto not be recognized in the entry workflowThis is because the
WorkflowDslclass used by the entry workflow must be augmented both with the typedparamsvariable (whenparamsblock is defined) and the piped operators (when static typing is not enabled)Due to code entropy (and lack of a unit test) these two behaviors accidentally clobbered each other. This PR fixes the issue and adds a regression test keep it fixed