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

fix: correct optional type handling in check_placeholder #345

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Ankush1oo8
Copy link

@Ankush1oo8 Ankush1oo8 commented Mar 8, 2025

Describe the problem or feature in addition to a link to the issues.

Before submitting this PR, please make sure:

  • Corrected the optional type check in check_placeholder function.

  • Ensured proper handling of Type::Primitive(_, bool).

  • Passed all tests successfully.

  • You have added a few sentences describing the PR here.

  • You have added yourself or the appropriate individual as the assignee.

  • You have added at least one relevant code reviewer to the PR.

  • Your code builds clean without any errors or warnings.

  • You have added tests (when appropriate).

  • You have updated the README or other documentation to account for these
    changes (when appropriate).

  • You have added an entry to the relevant CHANGELOG.md (see
    ["keep a changelog"] for more information).

  • Your commit messages follow the [conventional commit] style.

Fixes #342

@adthrasher
Copy link
Member

adthrasher commented Mar 10, 2025

@Ankush1oo8 - Please update your PR description to use the provided template: https://github.com/stjude-rust-labs/wdl/blob/main/.github/pull_request_template.md?plain=1. Please fill in the checklist also.

@adthrasher
Copy link
Member

@Ankush1oo8 - Also in the future, please comment on issues to declare interest and claim them prior to working on a PR. This allows to us avoid duplication of work.

@Ankush1oo8
Copy link
Author

@Ankush1oo8 - Also in the future, please comment on issues to declare interest and claim them prior to working on a PR. This allows to us avoid duplication of work.

Okey sir

@Ankush1oo8
Copy link
Author

@adthrasher i have removed all extra white spaces you told

Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Hi @Ankush1oo8, thanks very much for the PR!

Overall, I think this is a great first step towards fixing #342!

I've added some feedback comments from my review; would you mind looking them over and discussing them with me? Thanks!

@Ankush1oo8
Copy link
Author

Ankush1oo8 commented Mar 10, 2025

Hi @Ankush1oo8, thanks very much for the PR!

Overall, I think this is a great first step towards fixing #342!

I've added some feedback comments from my review; would you mind looking them over and discussing them with me? Thanks!

Yes understanding and implementing them

@Ankush1oo8
Copy link
Author

@peterhuene can you check now all test are clear

@Ankush1oo8 Ankush1oo8 requested a review from peterhuene March 11, 2025 08:32
Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Hi @Ankush1oo8. I've reviewed your changes and this is headed in the right direction, but there's a few comments I have. Thanks!

@@ -0,0 +1,9 @@
error: call to function `read_json` failed: failed to open file `attempts/0/work/not-array.json`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error should be different than this as this one is because I gave you a wrong example earlier.

I would expect a runtime error that matches exactly what would happen on analysis if the code had been {sep=',' true}.

However, there haven't been any changes to the expression evaluator in wdl-engine to do the type checks, which would be required to get the correct output here.

@Ankush1oo8
Copy link
Author

@peterhuene I will follow the suggestions Thanks

@Ankush1oo8
Copy link
Author

Ankush1oo8 commented Mar 13, 2025

@peterhuene hey Hi I have created he function as you said

pub(crate) fn check_placeholder(&mut self, placeholder: &Placeholder) {
        self.placeholders += 1;  // Track placeholder processing count
    
        let expr = placeholder.expr(); // Extract expression inside placeholder
    
        // Evaluate the placeholder expression and check that the resulting type is
        // coercible to string for interpolation
        if let Some(ty) = self.evaluate_expr(&expr) {
            match ty {
                // If the type is already coercible, do nothing
                Type::Primitive(..) | Type::Union | Type::None => {
                    // Validate primitive types with their options
                    if let Some(option) = placeholder.option() {
                        let valid = match option {
                            PlaceholderOption::Sep(_) => false, // Sep option is not valid for primitives
                            PlaceholderOption::Default(_) => matches!(ty, Type::Primitive(_, true)),
                            PlaceholderOption::TrueFalse(_) => matches!(ty, Type::Primitive(PrimitiveType::Boolean, _)),
                        };
                        
                        if !valid {
                            self.context.add_diagnostic(
                                Self::invalid_placeholder_option(&ty, expr.span(), Some(option))
                            );
                        }
                    } else {
                        // Apply cannot_coerce_to_string if no option is specified
                        self.context.add_diagnostic(cannot_coerce_to_string(&ty, expr.span()));
                    }
                }
    
                // Otherwise, check placeholder options
                _ => {
                    let coercible = match placeholder.option() {
                        // `Sep` option: Only `Array[P]` where `P` is a non-optional primitive or Union is valid
                        Some(PlaceholderOption::Sep(_)) => matches!(ty, 
                            Type::Compound(CompoundType::Array(ref element_type), false) // Enforcing non-optional Array
                            if matches!(element_type.element_type(), Type::Primitive(_, false) | Type::Union)),
    
                        // `Default` option: Only optional primitive types are valid
                        Some(PlaceholderOption::Default(_)) => matches!(ty, 
                            Type::Primitive(_, true)),
    
                        // `TrueFalse` option: Only Boolean type is valid
                        Some(PlaceholderOption::TrueFalse(_)) => matches!(ty, 
                            Type::Primitive(PrimitiveType::Boolean, _)),
    
                        None => false, // Apply cannot_coerce_to_string if no option is specified
                    };
                    if !coercible {
                        self.context
                            .add_diagnostic(
                                Self::invalid_placeholder_option(&ty, expr.span(), placeholder.option())
                                
                            );
                    }
                }
            }
        }
    
        self.placeholders -= 1;
    }

/// Generates a diagnostic message for invalid placeholder options.
fn invalid_placeholder_option(ty: &Type, span: Span, option: Option<PlaceholderOption>) -> Diagnostic {
    let message = match option {
        Some(PlaceholderOption::Sep(_)) => format!(
            "type mismatch for placeholder option `sep`: expected type `Array[P]` where P: any primitive type or Union, but found `{}`", ty
        ),
        Some(PlaceholderOption::Default(_)) => format!(
            "type mismatch for placeholder option `default`: expected an optional primitive type, but found `{}`", ty
        ),
        Some(PlaceholderOption::TrueFalse(_)) => format!(
            "type mismatch for placeholder option `truefalse`: expected type `Boolean`, but found `{}`", ty
        ),
        None => "Unexpected type for placeholder".to_string(),
    };
    
    return Diagnostic::error(message);
}

    

    /// Evaluates the type of a literal array expression.
    fn evaluate_literal_array(&mut self, expr: &LiteralArray) -> Type {
        // Look at the first array element to determine the element type
        // The remaining elements must have a common type
        let mut elements = expr.elements();
        match elements
            .next()
            .and_then(|e| Some((self.evaluate_expr(&e)?, e.span())))
        {
            Some((mut expected, mut expected_span)) => {
                // Ensure the remaining element types share a common type
                for expr in elements {
                    if let Some(actual) = self.evaluate_expr(&expr) {
                        if let Some(ty) = expected.common_type(&actual) {
                            expected = ty;
                            expected_span = expr.span();
                        } else {
                            self.context.add_diagnostic(no_common_type(
                                &expected,
                                expected_span,
                                &actual,
                                expr.span(),
                            ));
                        }
                    }
                }

                ArrayType::non_empty(expected).into()
            }
            // Treat empty array as `Array[Union]`
            None => ArrayType::new(Type::Union).into(),
        }
    }
but i dont know what to do the test as i am learning rust but not getting what to do according to your suggestion

@peterhuene
Copy link
Collaborator

Hi @Ankush1oo8. What you have there seems relatively correct, although I would probably restructure your check_placeholder to basically first check to see if there's an option:

If there is an option, follow the option's type requirements and emit a invalid_placeholder_option diagnostic if the expression type isn't what is expected.

If there is no option, only accept Type::Primitive(..) | Type::Union | Type::None, otherwise emit a cannot_coerce_to_string diagnostic.

Regarding testing, we can create a new source.wdl file under wdl-analysis/tests/analysis/placeholders that looks like this:

# This is a test for analysis of placeholders and their options.
version 1.1

workflow test {
    String s1 = "~{true}" # OK
    String s2 = "~{true="yes" false="no" false}" # OK
    String s3 = "~{true="yes" false="no" 1}" # NOT OK
    # More test cases here...
}

If we run cargo test -p wdl-analysis --all-features with the BLESS environment variable set to 1, it will generate a baseline file containing all of the expected diagnostics for the test case. We should then look the baseline over for correctness to ensure all of the diagnostics emitted for the test are what we expect.

@peterhuene
Copy link
Collaborator

@Ankush1oo8 Also, I merged in a refactoring which caused a conflict in wdl-analysis/src/types/v1.rs. If you need assistance with resolving the conflict, please let me know and I'm happy to help.

@Ankush1oo8
Copy link
Author

@peterhuene can you check now

@claymcleod claymcleod changed the title Fix: Correct optional type handling in check_placeholder fix: correct optional type handling in check_placeholder Mar 19, 2025
@claymcleod
Copy link
Member

Hey @Ankush1oo8, seems like this is still having a conflict (you can see at the bottom of the PR where it says "This branch has conflicts that must be resolved"). You should rebase your branch based on the latest main branch.

@claymcleod claymcleod added the S-awaiting-revisions State: awaiting revisions based on code review feedback label Mar 19, 2025
@Ankush1oo8
Copy link
Author

@claymcleod yes doing that

@Ankush1oo8 Ankush1oo8 requested a review from adthrasher March 19, 2025 21:06
@claymcleod
Copy link
Member

@Ankush1oo8 looks like there are now some errors in the CI, if you could please address those.

@peterhuene peterhuene self-requested a review March 21, 2025 15:28
Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Hi @Ankush1oo8. We're getting close now!

Just a few comments relating to the diagnostic and the check.

@@ -692,6 +693,33 @@ impl<'a, C: EvaluationContext> ExprTypeEvaluator<'a, C> {
self.placeholders -= 1;
}

fn invalid_placeholder_option(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move this function into diagnostics.rs, where we consolidate all diagnostic messages from static analysis.

@@ -0,0 +1,6 @@
error: type mismatch for placeholder option `sep`: expected type `Array[P]` where P: any primitive type or Union, but found `Boolean`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to regenerate the test baselines with BLESS=1 with the above changes to invalid_placeholder_option.

@@ -0,0 +1,13 @@
# This is a test for analysis of placeholders and their options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should merge this with the placeholder-options test.

@@ -0,0 +1,46 @@
#[cfg(test)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-commenting here as I think we can delete this file.

@peterhuene peterhuene added the S-awaiting-pass-CI State: awaiting the CI to pass label Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-pass-CI State: awaiting the CI to pass S-awaiting-revisions State: awaiting revisions based on code review feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spec compliance: placeholder options are not enforcing type requirements
4 participants