JavaSrc2Cpg: Infer type by Namespace and arg/parameter size

[khemrajrathore]

This PR has the following changes

• Currently in TypeInferencePass, a call is linked to a method If
  - Namespace matches and
  - Number of arguments in call and Number of parameter in method matches and
  - Type of arguments in call and Type of parameter in method matches
• If after applying these we are not able to link a call to a method, we try to link If
  - Namespace matches and
  - Number of argument in call and Number of parameter in method matches

[DavidBakerEffendi]

This type inference is meant to be more sound than type recovery, so an additional constraint I'd prefer you add is that, if we're ignoring argument types, that no other method has the same number of args, otherwise we may be calling either method.

[DavidBakerEffendi]

An iterator can only be used once, and I see it used twice. Rather get rid of candidateMethodsIter and just call candidateMethods.start or candidateMethods.iterator when you need it.

[khemrajrathore]

The reason for calling it twice is

• The first iterator call gives us the first match and the iterator stops there
• we use the same iterator to continue the search to see if we get another matching method

This is an optimization than calling the iterator from the start again

[DavidBakerEffendi]

No, iterators cannot be used more than once. I've verified this in a shell for you e.g.

scala> val x = Iterator(1, 2, 3)
val x: Iterator[Int] = <iterator>
                                                                                                                                                                                                                                                                                                                                                          
scala> x.toList
val res2: List[Int] = List(1, 2, 3)
                                                                                                                                                                                                                                                                                                                                                          
scala> x.toList
val res3: List[Int] = List()

[johannescoetzee]

This is intentional (maybe a comment is required in code to explain this) and is an optimisation to avoid having to traverse the entire iterator twice.

The first call to candidateMethodsIter.find consumes iterator elements until the first match, but stops at this point. The second find call then continues the search until a second match is found.

To illustrate with an example considering only the first find call:

scala> val x = Iterator(1, 2, 3, 2, 4)
val x: Iterator[Int] = <iterator>
                                                                                               
scala> x.find(_ == 2)
val res3: Option[Int] = Some(2)
                                                                                               
scala> x.toList
val res4: List[Int] = List(3, 2, 4)

And for both calls:

scala> val x = Iterator(1, 2, 3, 2, 4)
val x: Iterator[Int] = <iterator>
                                                                                               
scala> x.find(_ == 2)
val res5: Option[Int] = Some(2)
                                                                                               
scala> x.find(_ == 2)
val res6: Option[Int] = Some(2)
                                                                                               
scala> x.toList
val res7: List[Int] = List(4)

[khemrajrathore]

Agree, find won't consume the iterator fully. Instead for condition = true, find will return the matched item to the flatMap for further processing

[DavidBakerEffendi]

Ah now I understand, TDIL! Thanks

[khemrajrathore]

This type inference is meant to be more sound than type recovery, so an additional constraint I'd prefer you add is that, if we're ignoring argument types, that no other method has the same number of args, otherwise we may be calling either method.

Yes, this is already handled.

The pass will only proceed and infer if we find only a single method matching the criteria

[johannescoetzee]

The changes in this PR seem to achieve the goal they intend to, but this was actually the way it was done in the TypeInferencePass in the past and we added the type check because of issues with inherited, non-overridden methods not being considered. This contributed to the general problem of unresolved type issues being hidden.

[johannescoetzee]

@fabsx00 and @DavidBakerEffendi I think it would be good to have another discussion about type inference in javasrc2cpg. We've run into a couple of situations where legitimate bugs were hidden by various type inference mechanisms (which would've been discovered easily if the signatures contained <unresolved...

Ideally, the CPG created by javasrc2cpg would only contain type information we know from the JavaParserSymbolSolver, along with type info from imports, with everything else happening in optional passes after AST creation (which this PR would fit).

This is a discussion to have separate from this PR though.

[DavidBakerEffendi]

@khemrajrathore, based on @johannescoetzee's message, I think it would be faster to override this in Privado's side if this is an urgent feature.

[DavidBakerEffendi]

@johannescoetzee Another option we discussed was if we added a flag that allows for this behaviour to be enabled, and is off by default. Would this be a good compromise?

[johannescoetzee]

@DavidBakerEffendi @khemrajrathore An off-by-default flag would work, although in my opinion this would be a temporary measure until we reach a decision on how to handle inference in general. As such, I suggest adding it as a hidden flag with a description saying this is temporary to avoid issues if we want to remove it later