Add line/column/offset info to IMPORT nodes across frontends

[kanishks-io]

Adds source position (lineNumber, columnNumber, offset/offsetEnd) to Import nodes by switching frontends over to the shared newImportNode builder in x2cpg's AstNodeBuilder, instead of constructing NewImport() manually.

Frontends updated

• javasrc2cpg — addImportsToScope now uses newImportNode for both specific imports and
  the wildcard (*) import.
• php2cpg — astForUseUse now uses newImportNode, so use statements get a line number
  and offset (previously had neither).
• kotlin2cpg — astForImportDirective now uses newImportNode instead of manually setting
  lineNumber/columnNumber. Line/column were already correct here; this change is mainly for
  consistency with the other frontends and picks up offset/offsetEnd for free.
• jssrc2cpg — collapsed the two overloads of createImportNodeAndAttachToCall into one, since
  the (code, importedEntity, importedAs, call) overload had no other caller. It now builds the
  Import node via newImportNode using the import declaration node itself rather than deriving
  position from the associated require(...) call. This also fixes a pre-existing bug where
  columnNumber was mistakenly set from call.lineNumber.

[johannescoetzee]

Looks good, with only a minor style comment! As another comment, please check that each of these frontends has at least one test testing line number, column number, and offsets. I do see tests for all of these, but not all for each of the frontends.

I noticed there are still a few import properties that are being set after newImportNode (specifically isWildcard, isModuleImport, and isExplicit). I assume this is because the frontends using these properties weren't using the newImportNode method, so it wasn't updated when these properties were added. Could you please look at the newImportNode uses across joern and add all of these "extra" properties as parameters with sensible defaults (probably false in most cases)? I think this would best be done in a follow-up PR since it's not really related to adding the line/column/offset info.

[johannescoetzee]

I think this would be easier to read if you create separate variables for these. Before, we had .code(...), for example, to tell us what the big expression meant, but now we've lost that context until we use editor tools to see the signature of the newImportNode method.

Another option would be to used named arguments for these, so

newImportNode(
  code = ...,
  ...
)

although I tend to lean to the separate variable solution.

[bbrehm]

I think we can merge as is.

We could address the issue about reconstructed code fields along the way (either figure out a way to get that from the parser, or document the now-known flaw in a source-code comment and add a unit test that documents whether import foo.bar gets normalized to import foo.bar).

Otoh javasrc2cpg appears to do the right thing, this is more of a nitpick, and this PR doesn't introduce the problem.

[bbrehm]

Reconstructing a plausible semantically equivalent code field is not the same as producing the original code field. Say import foo.bar vs import foo.bar (one whitespace vs two whitespaces). This can fuck up people who copy-paste from the code field into grep.

If the kotlin parser doesn't give us the real code field, then this reconstruction is the best we can do. But it's not pretty. (also it's not an issue you introduced @kanishks-io )

[johannescoetzee]

The question of what the code field should actually represent is one with an inconsistent answer across time and frontends. It's not a huge issue for import nodes where whitespace is likely to be the only difference between our reconstruction of the code vs the original source code, but for constructs where we do more complex lowerings (lambda functions being one of the more extreme examples, but also foreach loops which we lower to traditional for-over-index loops in javasrc), we've been flip-flopping between using the code field to represent the original source vs some reconstruction from our lowerings.

To solve this problem, we extended the offset and offsetEnd properties to cover all AST nodes, so if you want to guarantee greppability, you have to run joern with --enable-file-content and use node.sourceCode instead of node.code.

There was also a plan to either remove the code field and replace it with a code method which would do some reconstruction of our lowering (or alternatively to keep the code field but populate it in some shared pass), although that's on hold indefinitely since some passes may rely on the current code fields making this a high risk for low potential reward change.

[kanishks-io]

Adding original ticket for tracking:
https://harness.atlassian.net/browse/QT-1494

[kanishks-io]

In Java and Kotlin both the extra spaces between "import foo.bar" are removed in the node.code property.
I have added UT to lockdown this behaviour, maybe we can have a discussion on how to handle this.
@bbrehm

[bbrehm]

I think you can merge, @johannescoetzee explained above.