[php2cpg] Php type recovery #3723
Conversation
|
@wunused Thanks for leading this effort, so far it's looking good. I am currently redesigning some parts of the type recovery abstract class to make it a bit more rigid and guaranteed, but will help you if it is merged before this PR goes through. |
|
@wunused, run |
Fixed, thanks! |
Pass will set the types for builtin functions with known function signatures. This pass will run before the PhpTypeRecovery pass. Currently, the pass does not handle variadic parameters.
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpSetKnownTypes.scala
Outdated
Show resolved
Hide resolved
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala
Outdated
Show resolved
Hide resolved
Currently the method return types are set correctly, but need to still update the parameter types.
This bug comes from the XTypeRecovery pass, but is fixed in PhpTypeRecovery by overriding the visitReturns method. If this approach (using another "symbol table" for methods) is sufficient, it should also be fixed abstract parent class.
There was a problem hiding this comment.
Great to see tests are passing! Added some suggestions. Additionally, could you add something like Php2Cpg.postProcessingPasses(cpg: Cpg)? See jssrc2cpg. This should help prevent duplication in PhpCode2CpgFixture as well as PhpCpgGenerator.
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpSetKnownTypes.scala
Outdated
Show resolved
Hide resolved
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpSetKnownTypes.scala
Outdated
Show resolved
Hide resolved
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala
Outdated
Show resolved
Hide resolved
| m: MethodRef, | ||
| rec: Option[String] = None | ||
| ): Set[String] = { | ||
| logger.debug(s"visiting identifier ${i.name} assigned to MethodRef ${m.code}") |
There was a problem hiding this comment.
If the sole purpose for overriding these functions is to set debugging info, I'd rather you set the logging in the base class
There was a problem hiding this comment.
I'll remove these from my pass and consider whether what the appropriate amount of logging is in the base class. These were helpful for me getting started with learning how the type recovery system worked, but they do clutter up the logging messages (unless that's acceptable for the debug log level?).
There was a problem hiding this comment.
Right, likely more a thing for TRACE. But either way it looks like this is more-or-less the last thing left before this is ready I'd say.
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/Php2Cpg.scala
Outdated
Show resolved
Hide resolved
This defines the list of default PHP postProcessingPasses in one location, so that they can all be applied without code duplication between the different places where passes are applied, like PhpCpgGenerator and PhpCode2CpgFixture. Additionally, the XTypeRecoveryConfig options are now exposed to the frontend command line arguments for PHP analysis as a result of this refactor.
|
@wunused I think let's get rid of the debugging, then it'll be ready to merge I'd say |
|
@DavidBakerEffendi sounds good - debug logging is removed and all tests are passing, so I'm marking this ready for review! |
| !name.isBlank && name.charAt(0).isUpper | ||
|
|
||
| override def assignments: Iterator[Assignment] = { | ||
| logger.debug( |
There was a problem hiding this comment.
Some debugging here still
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala
Outdated
Show resolved
Hide resolved
joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala
Outdated
Show resolved
Hide resolved
- Changed string equality comparisons from equals() to == - Changed "<module>" to "<global>" as top-level method namespace - Removed errant debug statement
|
@DavidBakerEffendi thanks, and comments are addressed! |
An implementation of a
PhpTypeRecoverymodule that extends theXTypeRecoveryabstract class to add some inferred types to PHP AST nodes.Any feedback is welcome as I continue work on this to ensure that the class is correct - I hope that this will be helpful for anyone else analyzing PHP programs.