Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report an error on duplicate workflow name definitions #4088

Merged
merged 6 commits into from
Sep 5, 2023

Conversation

bentsherman
Copy link
Member

Close #4086

Consider the following pipeline script:

// main.nf
include { WORK_ONE } from './w1'

workflow WORK_ONE {
    WORK_ONE ()
}

workflow {
    WORK_ONE ()
}
// w1.nf
workflow WORK_ONE {
    exit 0, "wf1"
}

Currently this causes a stack overflow error:

$ nextflow run gh-4086
N E X T F L O W  ~  version 23.06.0-edge
Launching `gh-4086/main.nf` [sick_williams] DSL2 - revision: c420b3da5f
ERROR ~ Unexpected error [StackOverflowError]

 -- Check script 'gh-4086/main.nf' at line: 4 or see '.nextflow.log' file for more details

With this PR, Nextflow will warn about the duplicate workflow name:

$ ../launch.sh run gh-4086
N E X T F L O W  ~  version 23.06.0-edge
Launching `gh-4086/main.nf` [jolly_avogadro] DSL2 - revision: db2d0fa85b
WARN: A process with name 'WORK_ONE' is defined more than once in module script: /home/bent/projects/nextflow-io_nextflow/sketches/gh-4086/main.nf -- Make sure to not define a function with the same name as a workflow or process
ERROR ~ Unexpected error [StackOverflowError]

 -- Check script 'gh-4086/main.nf' at line: 4 or see '.nextflow.log' file for more details

And if strict mode is enabled, this warning will become an error.

@bentsherman bentsherman requested a review from pditommaso July 11, 2023 14:28
@netlify
Copy link

netlify bot commented Jul 11, 2023

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit f8ecabb
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/64bff55684296400083924a6
😎 Deploy Preview https://deploy-preview-4088--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.

@pditommaso
Copy link
Member

It would be nice to add a unit test to validate this

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

If you define two functions with the same name with different signatures, can they both be used or only the last definition? That will affect whether we should return an error or only a warning if the duplicates are both functions.

@bentsherman
Copy link
Member Author

Did some testing, found that function overloading works the same as in Groovy as far as I can tell. If you define two functions with the same signatures, the Groovy compiler catches it before Nextflow. So checkComponentName() pertains mainly to checking for any duplicates introduced by module imports.

Since we changed it to always be an error, I had to remove a test that allows duplicate module imports.

Also, I think ScriptMeta::validate() is dead code. It checks for duplicate functions, but that should always be caught by checkComponentName() when the function is imported. I couldn't find a test case that triggered an error from validate().

@pditommaso
Copy link
Member

If I understand correctly, the exception is thrown irrespective of the strict flag, right?

@bentsherman
Copy link
Member Author

Correct

@pditommaso pditommaso changed the title Add warning about duplicate workflow definitions Report an error on duplicate workflow name definitions Sep 5, 2023
@pditommaso
Copy link
Member

Ok, update the PR title to reflect that

@pditommaso pditommaso merged commit fce9702 into master Sep 5, 2023
@pditommaso pditommaso deleted the 4086-fix-duplicate-workflow-name branch September 5, 2023 19:41
@pditommaso pditommaso added this to the 23.10.0 milestone Sep 10, 2023
abhi18av pushed a commit to abhi18av/nextflow that referenced this pull request Oct 28, 2023
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.

StackOverflowError when imported workflow has same name as native definition
2 participants