Skip to content

Change .location, NewLocation, and Location to lazily compute location information#5530

Merged
johannescoetzee merged 6 commits into
masterfrom
markus/lazyloc
Jun 26, 2025
Merged

Change .location, NewLocation, and Location to lazily compute location information#5530
johannescoetzee merged 6 commits into
masterfrom
markus/lazyloc

Conversation

@ml86
Copy link
Copy Markdown
Contributor

@ml86 ml86 commented Jun 12, 2025

This PR is a copy of #5482 which
got closed because Michael Flanders deleted the repository from which
he made the pull request.

  • Adds Loc class as replacement for LocationCreator and the location node step. Loc is a replacement that is meant to compute and store less up-front compared to LocationCreator and location. IIUC, scala lazy values also take up space, so I eyeballed the operations that might take more steps to compute and made those lazy and those that are just lookups on the underlying node and made these methods.
  • Adds a duplicate of one of the c2cpg test files to use Loc rather than replacing the other tests for now
  • Add deprecation warnings to the previous location code

@johannescoetzee johannescoetzee force-pushed the markus/lazyloc branch 4 times, most recently from 11ae016 to 46dcdf5 Compare June 25, 2025 10:15
Copy link
Copy Markdown
Contributor

@johannescoetzee johannescoetzee left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @ml86. From a Slack message from him:
Consider the joern PR approved. Since i opened it, you need to approve it

@johannescoetzee johannescoetzee merged commit efc42d0 into master Jun 26, 2025
8 checks passed
@johannescoetzee johannescoetzee deleted the markus/lazyloc branch June 26, 2025 13:05
mpollmeier added a commit that referenced this pull request Jul 30, 2025
prior to this change:
```
List((1,2,3)).toJson
// [{"head":1,"next":{"head":2,"next":{"head":3,"next":{}}}}]
```

now:
```
List((1,2,3)).toJson
// [1,2,3]
```

It's a regression that was introduced in #5530
I have no idea why Michael Flanders made this change.
This is also adding a test so we catch it early next time round.
mpollmeier added a commit that referenced this pull request Jul 30, 2025
* fix json serializer: define only for nodes, not for all Products

prior to this change:
```
List((1,2,3)).toJson
// [{"head":1,"next":{"head":2,"next":{"head":3,"next":{}}}}]
```

now:
```
List((1,2,3)).toJson
// [1,2,3]
```

It's a regression that was introduced in #5530
I have no idea why Michael Flanders made this change.
This is also adding a test so we catch it early next time round.

* fixup json serializer and tests

We don't want to serialize _all_ products, that's the main point of this
PR. But since Location is not a node and we still want to serialize it,
and it makes sense to have the product accessors, I added a marker trait
that let's us specifically format types using the product accessors...
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.

2 participants