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

[Ruby] do-end Blocks (with implicit &Proc and yield handling) #3928 #4359

Merged

Conversation

badly-drawn-wizards
Copy link
Contributor

@badly-drawn-wizards badly-drawn-wizards commented Mar 18, 2024

  • Adds yield RubyNode
  • Handles yield in RubyNodeCreator
  • Generates a ProcParameter with a unique identifier if a yield is used in a method with no block parameter
  • Generates a call to the block parameter for yields

@badly-drawn-wizards badly-drawn-wizards self-assigned this Mar 18, 2024
@badly-drawn-wizards badly-drawn-wizards added the ruby Relates to rubysrc2cpg label Mar 18, 2024
@badly-drawn-wizards badly-drawn-wizards force-pushed the 3928-ruby-do-end-blocks-with-and-yield-handling branch from bc8af93 to f9e6a51 Compare March 19, 2024 09:56
@badly-drawn-wizards badly-drawn-wizards marked this pull request as ready for review March 19, 2024 12:14
scope.useProcParam match {
case Some(param) =>
astForMemberCall(
MemberCall(SimpleIdentifier()(node.span.spanStart(param)), ".", "call", node.arguments)(node.span)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we lower this differently? My concern is that this is closer to reflection in how this is executed, and not sure that our call graph could resolve this.

Could this instead be a SimpleCall?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, concerning flows, the yield needs to map to MethodReturn, how is this done in pysrc2cpg/should we add a return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

      case node: ast.Yield          => unhandled(node)
      case node: ast.YieldFrom      => unhandled(node)

it isn't handled there, but they would need to be handled vastly differently anyway.

Copy link
Collaborator

@DavidBakerEffendi DavidBakerEffendi left a comment

Choose a reason for hiding this comment

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

Looks good, let's follow your suggestion (from our chat offline) to add some kind of custom conditional to handle the case where yields can return or generate a collection, etc.

@badly-drawn-wizards badly-drawn-wizards merged commit 4a12a27 into master Mar 20, 2024
5 checks passed
@DavidBakerEffendi DavidBakerEffendi deleted the 3928-ruby-do-end-blocks-with-and-yield-handling branch March 20, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Relates to rubysrc2cpg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ruby] do-end Blocks (with implicit &Proc and yield handling)
2 participants