Skip to content

Ruby: Avoid computing full fastTC for AstNode::getParent #10741

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 10, 2022
Merged

Ruby: Avoid computing full fastTC for AstNode::getParent #10741

merged 4 commits into from
Oct 10, 2022

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 9, 2022 •
edited
Loading

Rewritten to use explicit recursion instead of (implicit) fastTC. See each commit for details.

hvitved added 4 commits October 9, 2022 11:10
@hvitved
Ruby: Avoid computing full fastTC for AstNode::getParent
d707c52 
DIL before
```
                                  /* AST::AstNode */ AST#87953007::Cached::TAstNode result) =
  fastTC(Module#fe82a56b::parent#1#ff/2)
.

Module#fe82a56b::enclosingModule#1#ff(/* AST::AstNode */ AST#87953007::Cached::TAstNode node,
                                      /* Module::ModuleBase */ AST#87953007::Cached::TAstNode result)
:-
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    Module#2a43f566::ModuleBase#f(result),
    project#AST#a6718388::AstNode::getAChild#1#dispred(result, call_result#2),
    (
      node = call_result#2;
      #Module#fe82a56b::parent#1Plus#ff(node, call_result#2)
    )
  )
.
```

DIL after
```
incremental
Module#fe82a56b::enclosingModule#1#ff(/* AST::AstNode */ AST#87953007::Cached::TAstNode node,
                                      /* Module::ModuleBase */ AST#87953007::Cached::TAstNode result)
:-
  (
    Module#2a43f566::ModuleBase#f(result),
    exists(cached dontcare string _ |
      AST#a6718388::AstNode::getAChild#1#dispred(result, _, node)
    )
  );
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode mid |
    Module#2a43f566::ModuleBase#f(result),
    rec Module#fe82a56b::enclosingModule#1#ff(mid, result),
    not(Module#2a43f566::ModuleBase#f(mid)),
    not(Method#8b49e67f::Block#f(mid)),
    exists(cached dontcare string _ |
      AST#a6718388::AstNode::getAChild#1#dispred(mid, _, node)
    )
  )
| [base_case]
  Module#2a43f566::ModuleBase#f(result),
  project#AST#a6718388::AstNode::getAChild#1#dispred(result, node)
| [delta_order]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode mid |
    Module#2a43f566::ModuleBase#f(result),
    delta previous rec Module#fe82a56b::enclosingModule#1#ff(mid, result),
    not(Module#2a43f566::ModuleBase#f(mid)),
    not(Method#8b49e67f::Block#f(mid)),
    project#AST#a6718388::AstNode::getAChild#1#dispred(mid, node)
  ),
  not(previous rec Module#fe82a56b::enclosingModule#1#ff(node, result))
| [delta_order_up_to_500000]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode mid |
    delta previous rec Module#fe82a56b::enclosingModule#1#ff(mid, result),
    Module#2a43f566::ModuleBase#f(result),
    not(Module#2a43f566::ModuleBase#f(mid)),
    not(Method#8b49e67f::Block#f(mid)),
    project#AST#a6718388::AstNode::getAChild#1#dispred(mid, node)
  ),
  not(previous rec Module#fe82a56b::enclosingModule#1#ff(node, result))
.
```
@hvitved
Ruby: Avoid computing full fastTC for AstNode::getParent
262a74d 
DIL before
```
                                                   /* AST::AstNode */ AST#87953007::Cached::TAstNode result) =
  fastTC(AST#a6718388::AstNode::getAChild#0#dispred#ff/2)
.

Completion#445d5844::mayRaise#1#f(/* Call::Call */ unique AST#87953007::Cached::TAstNode c)
:-
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    exists(/* ControlFlowGraphImpl::Trees::BodyStmtTree */ AST#87953007::Cached::TAstNode bst |
      (
        (
          project#Expr#6fb2af19::BodyStmt::getRescue#1#dispred#fff(bst),
          ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
        );
        (
          exists(/* Expr::StmtSequence */ dontcare AST#87953007::Cached::TAstNode _ |
            Expr#6fb2af19::BodyStmt::getEnsure#0#dispred#ff(bst, _)
          ),
          ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
        )
      ),
      ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst),
      project#ControlFlowGraphImpl#288ae92e::Trees::StmtSequenceTree::getBodyChild#2#dispred#ffff(bst,
                                                                                                  call_result#2)
    ),
    (
      (c = call_result#2, Call#841c84e8::Call#f(c));
      (
        #AST#a6718388::AstNode::getAChild#0#dispredPlus#ff(call_result#2, c),
        Call#841c84e8::Call#f(c)
      )
    )
  )
.
```

DIL after
```
incremental
Completion#445d5844::getARescuableBodyChild#0#f(/* AST::AstNode */ unique AST#87953007::Cached::TAstNode result)
:-
  exists(/* ControlFlowGraphImpl::Trees::BodyStmtTree */ AST#87953007::Cached::TAstNode bst |
    (
      (
        exists(dontcare int _,
               /* Expr::RescueClause */ dontcare AST#87953007::Cached::TAstNode _1 |
          Expr#6fb2af19::BodyStmt::getRescue#1#dispred#fff(bst, _, _1)
        ),
        ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
      );
      (
        exists(/* Expr::StmtSequence */ dontcare AST#87953007::Cached::TAstNode _ |
          Expr#6fb2af19::BodyStmt::getEnsure#0#dispred#ff(bst, _)
        ),
        ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
      )
    ),
    ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst),
    exists(boolean arg2, dontcare int _ |
      arg2 = true,
      ControlFlowGraphImpl#288ae92e::Trees::StmtSequenceTree::getBodyChild#2#dispred#ffff(bst,
                                                                                          _,
                                                                                          arg2,
                                                                                          result)
    )
  );
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#5 |
    rec Completion#445d5844::getARescuableBodyChild#0#f(call_result#5),
    exists(cached dontcare string _ |
      AST#a6718388::AstNode::getAChild#1#dispred(call_result#5, _, result)
    )
  )
| [base_case]
  exists(/* ControlFlowGraphImpl::Trees::BodyStmtTree */ AST#87953007::Cached::TAstNode bst |
    (
      (
        project#Expr#6fb2af19::BodyStmt::getRescue#1#dispred#fff(bst),
        ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
      );
      (
        exists(/* Expr::StmtSequence */ dontcare AST#87953007::Cached::TAstNode _ |
          Expr#6fb2af19::BodyStmt::getEnsure#0#dispred#ff(bst, _)
        ),
        ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst)
      )
    ),
    ControlFlowGraphImpl#288ae92e::Trees::BodyStmtTree#class#f(bst),
    project#ControlFlowGraphImpl#288ae92e::Trees::StmtSequenceTree::getBodyChild#2#dispred#ffff(bst,
                                                                                                result)
  )
| [delta_order]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#5 |
    delta previous rec Completion#445d5844::getARescuableBodyChild#0#f(call_result#5),
    project#AST#a6718388::AstNode::getAChild#1#dispred(call_result#5, result)
  ),
  not(previous rec Completion#445d5844::getARescuableBodyChild#0#f(result))
.
```
@hvitved
Ruby: Avoid computing full fastTC for AstNode::getParent
d39b0fd 
DIL before
```
                                                   /* AST::AstNode */ AST#87953007::Cached::TAstNode result) =
  fastTC(AST#a6718388::AstNode::getAChild#0#dispred#ff/2)
.

Synthesis#d9ff06b1::isInDesugaredContext#1#f(/* AST::AstNode */ unique AST#87953007::Cached::TAstNode n)
:-
  exists(int arg1, /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
    arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, n)
  );
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, call_result#2)
    ),
    #AST#a6718388::AstNode::getAChild#0#dispredPlus#ff(call_result#2, n)
  )
.
```

DIL after
```
incremental
Synthesis#d9ff06b1::isInDesugaredContext#1#f(/* AST::AstNode */ unique AST#87953007::Cached::TAstNode n)
:-
  exists(int arg1, /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
    arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, n)
  );
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode any#expr##2 |
    rec Synthesis#d9ff06b1::isInDesugaredContext#1#f(any#expr##2),
    exists(cached dontcare string _ |
      AST#a6718388::AstNode::getAChild#1#dispred(any#expr##2, _, n)
    )
  )
| [base_case]
  exists(int arg1, /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
    arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, n)
  )
| [delta_order]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode any#expr##2 |
    delta previous rec Synthesis#d9ff06b1::isInDesugaredContext#1#f(any#expr##2),
    project#AST#a6718388::AstNode::getAChild#1#dispred(any#expr##2, n)
  ),
  not(previous rec Synthesis#d9ff06b1::isInDesugaredContext#1#f(n))
.
``
@hvitved
Ruby: Avoid computing full fastTC for AstNode::getParent
02192ac 
DIL before
```
                                                   /* AST::AstNode */ AST#87953007::Cached::TAstNode result) =
  fastTC(AST#a6718388::AstNode::getAChild#0#dispred#ff/2)
.

Synthesis#d9ff06b1::Desugared::getADescendant#0#dispred#ff(/* Synthesis::Desugared */ AST#87953007::Cached::TAstNode this,
                                                           /* AST::AstNode */ AST#87953007::Cached::TAstNode result)
:-
  (
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    result = this
  );
  (
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    #AST#a6718388::AstNode::getAChild#0#dispredPlus#ff(this, result)
  )
.
```

DIL after
```
incremental
Synthesis#d9ff06b1::Desugared::getADescendant#ff(/* Synthesis::Desugared */ AST#87953007::Cached::TAstNode this,
                                                 /* AST::AstNode */ AST#87953007::Cached::TAstNode result)
:-
  (
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    result = this
  );
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    rec Synthesis#d9ff06b1::Desugared::getADescendant#ff(this, call_result#2),
    exists(cached dontcare string _ |
      AST#a6718388::AstNode::getAChild#1#dispred(call_result#2, _, result)
    )
  )
| [base_case]
  exists(int arg1, /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
    arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
  ),
  result = this
| [delta_order]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    delta previous rec Synthesis#d9ff06b1::Desugared::getADescendant#ff(this,
                                                                        call_result#2),
    project#AST#a6718388::AstNode::getAChild#1#dispred(call_result#2, result)
  ),
  not(
    previous rec Synthesis#d9ff06b1::Desugared::getADescendant#ff(this, result)
  )
| [delta_order_up_to_500000]
  exists(/* AST::AstNode */ AST#87953007::Cached::TAstNode call_result#2 |
    delta previous rec Synthesis#d9ff06b1::Desugared::getADescendant#ff(this,
                                                                        call_result#2),
    exists(int arg1,
           /* AST::AstNode */ dontcare AST#87953007::Cached::TAstNode _ |
      arg1 = -1, AST#87953007::Cached::getSynthChild#2(_, arg1, this)
    ),
    project#AST#a6718388::AstNode::getAChild#1#dispred(call_result#2, result)
  ),
  not(
    previous rec Synthesis#d9ff06b1::Desugared::getADescendant#ff(this, result)
  )
.
```
@github-actions github-actions bot added the Ruby label Oct 9, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 9, 2022
View reviewed changes
ruby/ql/lib/codeql/ruby/ast/internal/Module.qll
result = n.getParent() and
not result instanceof ModuleBase and
not result instanceof Block
private ModuleBase enclosingModule(AstNode node) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter

The QLDoc has no documentation for node, but the QLDoc mentions module_eval
@hvitved hvitved changed the title Ruby: Avoid computing full fastTC for AstNode::getParent Ruby: Avoid computing full fastTC for AstNode::getParent Oct 9, 2022
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Oct 9, 2022
@hvitved hvitved marked this pull request as ready for review October 9, 2022 12:55
@hvitved hvitved requested a review from a team as a code owner October 9, 2022 12:55
@calumgrant calumgrant requested a review from nickrolfe October 10, 2022 08:47
nickrolfe
nickrolfe approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

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

LGTM

@hvitved hvitved merged commit 099251a into github:main Oct 10, 2022
@hvitved hvitved deleted the ruby/no-full-fast-tc branch October 10, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickrolfe nickrolfe nickrolfe approved these changes

Assignees
No one assigned
Labels
no-change-note-required This PR does not need a change note Ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hvitved @nickrolfe