Skip to content

Conversation

bentsherman
Copy link
Member

@bentsherman bentsherman commented Dec 21, 2023

Implements the strict syntax for Nextflow scripts, using the shared "compiler" module from the language server.

To use the strict syntax, simply set NXF_ENABLE_STRICT_SYNTAX=true in your environment when running nextflow.

This approach allows us to control the parsing process -- including the syntax and detecting syntax errors -- while still leveraging the Groovy compiler for execution. In other words, we can define whatever grammar we want, as long as we can "compile" it into a Groovy AST. If you look at ScriptToGroovyVisitor, you'll see that it converts processes / workflows / includes into the same Groovy AST produced by NextflowDSLImpl.

NOTE: While this PR uses Jitpack to load the shared module, this dependency should be inverted before the release of 25.04. That is, eventually the compiler module should reside in Nextflow and the language server should consume it.

Copy link

netlify bot commented Dec 21, 2023

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit cdbe37e
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/67d07f026aff76000813f819
😎 Deploy Preview https://deploy-preview-4613--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bentsherman
Copy link
Member Author

Some sweets-infused holiday thoughts... right now I am just producing the same AST expected by the runtime to keep this PR as simple as possible. But, like I said, we can produce whatever Groovy AST we want, so we could produce Groovy code that more effectively enables new features like static types, default arguments, etc.

The main example I'm thinking of is the annotation API (see nextflow-io/rnaseq-nf#24). I originally designed it as user-facing code, but it could also be an intermediate representation that is produced by the parser. If we "compile" the process and workflow definitions to actual function definitions, then we can more easily leverage the Groovy type checking.

This is just an example. We may not need the annotation API exactly, but it would be good to explore alternative AST representations, perhaps in a second iteration.

Signed-off-by: Ben Sherman <[email protected]>
@bentsherman
Copy link
Member Author

Another aside... GraalVM implements an AST model for every language that it supports. Here is the Graal Python AST source code.

So we could also have the parser produce a Graal/Python AST and thereby allow the pipeline code to use Python semantics instead of Groovy semantics.

We would need to design a DSL syntax for processes and workflows that would make sense with Python. Likely it would look more like Snakemake. Using native Python syntax (i.e. functions with decorators) is also an option but would likely be more verbose. We would still need to implement our own IDE tooling, but centered around Python syntax instead of Groovy syntax.

The point is, if we rely on the semantics (and compiler backend) of an existing language, it doesn't have to be Groovy. It could easily be any language supported by GraalVM.

Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman changed the title Nextflow parser Formal grammar and parser Apr 29, 2024
@bentsherman bentsherman changed the title Formal grammar and parser Script parser Jul 25, 2024
@bentsherman
Copy link
Member Author

it would be preferable reduce the deltas across version

I renamed the tests in both folders, so they should be the same? What does the directory comparison look like?

@pditommaso
Copy link
Member

I renamed the tests in both folders, so they should be the same?

Yes, it would be better

What does the directory comparison look like?

Screenshot 2025-03-10 at 16 24 58

@bentsherman
Copy link
Member Author

Are you comparing different branches? My directory compare looks like this (I have a few local changes too):

image

@bentsherman bentsherman marked this pull request as ready for review March 10, 2025 16:00
@bentsherman
Copy link
Member Author

Regarding the IntelliJ errors, I have IntelliJ open but I am not seeing them. Are they preventing you from building?

IntelliJ's Groovy analyzer does not seem to like these lambdas, though I don't know why, because they are correct

If the IntelliJ errors aren't blocking, what I'd rather do is merge this one and in a separate PR, port ScriptCompiler back to Java, because I know the Java compiler won't have those errors. I just might need to adjust some underlying Groovy classes like BaseScript to make it work

Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not even compiling for me

Apart this I don't really feel comfortable merging having IDE syntax errors, especially in such central component (compiler)

Comment on lines 206 to 210
// skip main script on second conversion pass
if( phase == Phases.CONVERSION && source == entry )
return
op.call(source)
}, phase)
Copy link
Member

@pditommaso pditommaso Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the IDE is correct to report an ambiguity. Even if it compiles it's not clear what method is invoking

Screenshot 2025-03-10 at 18 20 41

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just how lambdas work in Java. Here's an example from Groovy's CompilationUnit.

The parameter type is ISourceUnitOperation, which is a functional interface. It defines a single abstract method which the lambda is matched against.

This would compile in Java. If IntelliJ is reporting it then it's either being paranoid or just incorrect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out how to keep the script compiler in Java, so these IntelliJ errors should go away.

I had to re-do some commits to fix the DCO, so you might have to reset and re-pull

git reset --hard 6cd60d1
git pull

@pditommaso
Copy link
Member

After refreshing my copy, I can compile.

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman
Copy link
Member Author

Tests are passing! Approve when you're ready and I'll merge it

@pditommaso
Copy link
Member

I'll check by today

@pditommaso
Copy link
Member

i've very confused, I don't see the updates on this branch

» git log 
e6b4048b - (HEAD -> ben-config-parser, origin/ben-config-parser) Minor change [ci fast] (2 weeks ago) <Paolo Di Tommaso>
53565bcf - Rename NXF_ENABLE_STRICT_SYNTAX -> NXF_SYNTAX_PARSER(=v2) (2 weeks ago) <Ben Sherman>
df99b11f - Separate ClosureToStringVisitor from xform (2 weeks ago) <Ben Sherman>
5511f655 - Refactor legacy/impl -> v1/v2 (2 weeks ago) <Ben Sherman>
ca26e26e - Merge branch 'master' into ben-config-parser (2 weeks ago) <Paolo Di Tommaso>

» git log origin/ben-config-parser
- Minor change [ci fast] [e6b4048b]
- Rename NXF_ENABLE_STRICT_SYNTAX -> NXF_SYNTAX_PARSER(=v2) [53565bcf]
- Separate ClosureToStringVisitor from xform [df99b11f]
- Refactor legacy/impl -> v1/v2 [5511f655]
- Merge branch 'master' into ben-config-parser [ca26e26e]
- cleanup [09f1498b]
- Fix Prevent S3 global option when using custom endpoints (#5779) [ed9da469]
- Fix dynamic publish path (#5809) [6a81c015]

@bentsherman
Copy link
Member Author

It should be origin/ben-custom-parser

@pditommaso
Copy link
Member

@pditommaso
Copy link
Member

pditommaso commented Mar 11, 2025

Ok, just reverted old tests nomenclature Let's stay with the cleaned version 👍

@bentsherman bentsherman merged commit 7e3868d into master Mar 11, 2025
22 of 23 checks passed
@bentsherman bentsherman deleted the ben-custom-parser branch March 11, 2025 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typo in the container declaration leads to null and nextflow using a local tool installation
3 participants