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

feat: for each loops #911

Merged
merged 20 commits into from
May 31, 2023
Merged

feat: for each loops #911

merged 20 commits into from
May 31, 2023

Conversation

zolamk
Copy link
Contributor

@zolamk zolamk commented May 25, 2023

Description

Introduces support for forEach loops in reviewpad DSL.

Summary generated by Reviewpad on 31 May 23 10:28 UTC

This pull request contains several changes across the codebase. The env.go and typeinfer.go files have modifications to support built-ins and check for their existence. The engine package has several changes, including new imports, implementations of built-in functions, and modifications to structs and methods to support new features. A new file reviewpad_with_conditional_foreach_workflow.yml has been added, which defines a group and workflow, including a conditional statement. Another new file reviewpad_with_foreach_workflow.yml defines a workflow that iterates over a list of teams and adds a label to each. The lintShadowedVariables() function has been added to handle variable shadowing in workflow runs and workflows. Finally, the inline_rules_normalized.go and exec.go files have changes to support forEach blocks and new package imports.

🤖 Generated by Copilot at eb1dd26

This pull request adds support for forEach clauses in run blocks, using the lang package from reviewpad/v4 to handle list expressions and values. It updates the engine, the linter, and the aladino interpreter to parse, validate, normalize, transform, and execute the forEach clauses. It also adds a Type method to the Value interface and its implementations, and enables type inference and evaluation for list expressions and temporary variables in the aladino language. It modifies several files in the engine and lang/aladino packages, and refactors some test functions in lang/aladino/interpreter_internal_test.go.

Code review and merge strategy

Ask: this pull request requires a code review before merge

How

🤖 Generated by Copilot at eb1dd26

  • Import lang package from reviewpad/v4 to use lang.Value interface and other types (link, link, link)
  • Add ProcessList and StoreTemporaryVariable methods to Interpreter interface to evaluate and store list expressions and temporary variables in engine environment (link)
  • Enable evaluation and type inference for registered values in aladino interpreter, by checking the register map before looking for built-in functions and types (link, link)
  • Implement Type method for all value types in aladino interpreter, returning the corresponding type instances (link, link, link, link, link, link, link)
  • Add ForEach field to PadWorkflowRunBlock struct, holding a pointer to PadWorkflowRunForEachBlock struct, which has fields for value, in, and do of forEach clause (link)
  • Add logic to normalize, transform, and execute forEach clause in run block, by validating the fields, calling normalizeRun and transformRunBlock recursively on do block, and iterating over the list expression and executing the do block for each value (link, link, link, link)
  • Add validation for run block to check if it has a forEach clause, and return an error if it has no then block, no actions, no extra actions, and no forEach block (link, link)
  • Rename evalGroup function to evalList in aladino interpreter, and update error messages and test functions to use the term list instead of group (link, link, link, link, link, link)
  • Loop over the register map of the environment and add the types of the registered values to the built-in types map, to enable type inference for the registered values (link)

@reviewpad reviewpad bot added the ask Pull request requires a code review before merge label May 25, 2023
Base automatically changed from refactor/lang to main May 26, 2023 11:13
@zolamk zolamk marked this pull request as ready for review May 26, 2023 11:46
@reviewpad reviewpad bot requested a review from marcelosousa May 26, 2023 11:46
@reviewpad reviewpad bot added large large pull request waiting-approval Pull request is waiting for approval labels May 26, 2023
engine/transform.go Outdated Show resolved Hide resolved
@marcelosousa
Copy link
Member

@zolamk I need some time to go over these changes. Please bare with me.

@reviewpad reviewpad bot added the requires-author-attention Pull request requires the author to take action label May 26, 2023
@reviewpad
Copy link

reviewpad bot commented May 26, 2023

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

@marcelosousa
Copy link
Member

/reviewpad summarize

Copy link
Member

@marcelosousa marcelosousa left a comment

Choose a reason for hiding this comment

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

Some minor tweaks - the big issue is that we need variable scoping.

The following program will "break":

groups:
  - name: test1
    spec: '["team1", "team2"]'
  - name: test2
    spec: '["team3", "team4"]'


workflows:
  - name: test
    run:
      - forEach:
          value: $teamName
          in: $group("test1")
          do: 
            - forEach:
                value: $teamName
                in: $group("test2")
                do: $addLabel($teamName)
            - $addLabel($teamName)

engine/lang.go Show resolved Hide resolved
engine/exec.go Outdated Show resolved Hide resolved
engine/exec.go Outdated Show resolved Hide resolved
engine/exec.go Outdated Show resolved Hide resolved
engine/exec.go Outdated Show resolved Hide resolved
@reviewpad
Copy link

reviewpad bot commented May 30, 2023

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

engine/exec.go Outdated
Comment on lines 418 to 438
if run.ForEach != nil {
var executedActions []string

value, err := interpreter.ProcessList(run.ForEach.In)
if err != nil {
return ExitStatusFailure, nil, err
}

// create a new execution scope for the for-each block
// so that the temporary variables are not shadowed
scopedInterpreter := interpreter.NewExecutionScope()

for _, val := range value.(*lang.ArrayValue).Vals {
scopedInterpreter.StoreTemporaryVariable(run.ForEach.Value, val)

exitStatus, forEachActions, err := execStatementBlock(scopedInterpreter, run.ForEach.Do, rules)
executedActions = append(executedActions, forEachActions...)
if err != nil {
return exitStatus, executedActions, err
}
}

return ExitStatusSuccess, executedActions, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

/reviewpad explain

Copy link

Choose a reason for hiding this comment

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

This code block is a part of the evaluation and execution of the workflows defined in the configuration file. The specific part you highlighted is responsible for handling the forEach construct within the workflow run block. It iterates over the values specified in the "in" parameter of the forEach construct, creates a new execution scope for the interpreter to prevent any variable shadowing, and then processes the nested statements inside the "do" block of the forEach construct.

The forEach construct allows iterating through lists of values and performing some actions based on each value in that list while keeping the temporary variables within their own execution scope. This code block is responsible for controlling this iteration and maintaining the correct execution scope while going through the list of values and executing the statements in the nested "do" block.

Copy link
Member

@marcelosousa marcelosousa left a comment

Choose a reason for hiding this comment

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

Some comments. For the TODOs, it is okay at this stage to simply open issues for follow-up to not block this PR (up to you to decide).

engine/exec.go Outdated Show resolved Hide resolved
lang/aladino/eval.go Show resolved Hide resolved
lang/value.go Show resolved Hide resolved
@reviewpad
Copy link

reviewpad bot commented May 31, 2023

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

@reviewpad reviewpad bot removed requires-author-attention Pull request requires the author to take action waiting-approval Pull request is waiting for approval labels May 31, 2023
Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

Pull request is ready to be merged. Reviewpad will merge it for you.

@reviewpad reviewpad bot merged commit c5a4d1a into main May 31, 2023
3 checks passed
@reviewpad reviewpad bot deleted the feat/array branch May 31, 2023 11:59
@zolamk
Copy link
Contributor Author

zolamk commented May 31, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ask Pull request requires a code review before merge large large pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants