Skip superfluous and error-prone variable reference check. #222
Conversation
|
👍 |
| // the namespace's descendants: once with a parent that is the containing static workflow and again with a parent that is the dynamic workflow | ||
| // call. Looking for variable references when the parent is a workflow call will fail if the variable references a call output since only | ||
| // declarations are made children of workflow calls. This variable reference check is also redundant to the check that will happen with the | ||
| // copy of the declaration that has a static workflow parent. |
There was a problem hiding this comment.
This look like it should be a single javadoc style comment since it should be considered attached to the parentIsNotAWorkflowCall method
There was a problem hiding this comment.
This is an internal function and wouldn't render as Scaladoc, but I could certainly use the multi-line syntax if that was deemed more palatable.
There was a problem hiding this comment.
Despite my previous thumb I'll chime in to say that I concur in that I prefer
/*
foo
*/
for multiline comments. I realize that wasn't a multiline comment :)
| // call. Looking for variable references when the parent is a workflow call will fail if the variable references a call output since only | ||
| // declarations are made children of workflow calls. This variable reference check is also redundant to the check that will happen with the | ||
| // copy of the declaration that has a static workflow parent. | ||
| def parentIsNotAWorkflowCall(declaration: DeclarationInterface): Boolean = declaration.parent.collect({ case p: WdlWorkflowCall => p }).isEmpty |
There was a problem hiding this comment.
filterByType[WdlWorkflowCall]
There was a problem hiding this comment.
Re previous conversation, since it's an Option I think I'd lean towards a match here after all
| val invalidVariableReferences = for { | ||
| expr <- declaration.expression.toSeq | ||
| variable <- referencesToAbsentValues(declaration, expr) | ||
| variable <- referencesToAbsentValues(declaration, expr) if parentIsNotAWorkflowCall(declaration) |
There was a problem hiding this comment.
Could you do this check on line 344 (before we even call validateDeclaration) to avoid wasting the effort of re-validating any statically-validated declarations?
There was a problem hiding this comment.
Looks like it, I can try.
|
@cjllanwarne ready for final review. |
mcovarr
force-pushed
the
mlc_subworkflow_var_refs
branch
from
f1de81e
to
d4ad7df
Compare
|
@Horneth I created a Centaur test for this. |
Addresses the issue found in this forum post.