nextflow-io / language-server

The Nextflow language server
Apache License 2.0
9 stars 0 forks source link

Declare process/workflow inputs as fields #29

Closed bentsherman closed 2 months ago

bentsherman commented 2 months ago

In Groovy, a local variable can have the same name as a class member, likely because class members are stored on the heap while local variables are stored on the stack.

Because process and workflow inputs in Nextflow are injected via closure delegate, they are effectively class members, which means they can be overridden by local variables in the same way:

workflow PREPARE_GENOME {
  take:
  fasta

  main:
  def fasta = '...' // redeclare as local variable

  Channel.of( 1, 2, 3 )
    .map { fasta -> fasta } // redeclare as closure parameter
    .view()
}

This is not allowed in a function, since a function parameter is just another local variable on the stack and would be clobbered:

def func(fasta) {
  def fasta = '...' // not allowed
  def cl = { fasta -> // not allowed
    // ...
  }
}

As a result, many Nextflow pipelines have gotten into the habit of re-declaring local variables over process/workflow inputs. It doesn't cause any issues during execution, although it might be confusing to read.

However, the language server currently declares process/workflow inputs as if they were function parameters, not class members, which means that all of these re-declared local variables are errors.

This PR makes the language server more aligned with current Nextflow semantics, which will reduce the number of errors for some users.

But first, I think we should consider -- do we want these semantics? Right now, the rules around variable scopes are unclear because it's just "whatever Groovy does". If we want to be more clear/strict, now might be the best time to enforce it, while we are clarifying other aspects of the language.

Some other scattered thoughts:

bentsherman commented 2 months ago

Closing based on today's discussion: