Skip to content

[php] Add <tmp> assignment chain for call chains / property accesses #5434

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AndreiDreyer
Copy link
Contributor

@AndreiDreyer AndreiDreyer commented Apr 11, 2025

Attempting this issue revealed a number of things that were incorrectly implemented and that had to be fixed as a prerequisite:

  • Fixed shadowing in the test suite of builtin_functions.txt in the resources
  • Changed how calls are represented - these include a base and receiver in a similar manner to other interpreted languages in Joern
  • Due to the nature of PHP, unless a dynamic call is dispatched within the scope of a class, there will not be a $this receiver.
  • Built-in functions are statically dispatched
  • Field accesses were found to use an identifier in place of a field identifier; this was rectified
  • Chained calls use aliasing with temporary variables to ensure that field accesses and calls only occur once, as per the source code.

TODO:

  • This broke some type recovery tests and may impact downstream users.
  • The way receivers may often be an identifier referring to the call name, the local pass created a local for these - investigate if this is canonical
  • Classes need to be split into an instance class and a singleton "meta" class, due to how object instances carry references to static functions. This needs to be referred to when static calls are made on object instances.
  • Call bindings need to be created where possible.

@DavidBakerEffendi DavidBakerEffendi marked this pull request as ready for review April 23, 2025 10:40
@DavidBakerEffendi DavidBakerEffendi requested a review from ml86 April 23, 2025 11:02
Comment on lines 31 to 33
base.name shouldBe Operators.fieldAccess
base.code shouldBe "$a->contents"
base.argumentIndex shouldBe -1
Copy link
Contributor

Choose a reason for hiding this comment

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

There must have been a misunderstanding in our last talk.
The base is the "this" argument and this is just an IDENTIFIER a in this case.
You can reference the same IDENTIFIER node as receiver.
I just red through the PHP docu and played around with it a little bit and i think we can model the PHP classes and the call dispatch in a classic vtable style.

base.code shouldBe "$a->contents"
base.argumentIndex shouldBe -1

receiver.name shouldBe "a"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check explicitly for the existence of the receiver.

@@ -187,4 +219,38 @@ class CallTests extends PhpCode2CpgFixture {
val call = cpg.call("test").head
call.code shouldBe "test(...$args)"
}

"chained calls should alias calls in receivers and bases" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

The hole tmp variable creation can be removed because in PHP the receiver and the this argument are always the same and you can just reference the same AST subtree.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, should we remove the base altogether? From what I understand, if the base and receiver are the same AST subtree the tmp stuff is there to reduce duplication between the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add one tree for the this argument and point to one argument and one receiver edge to it. No extra base tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants