Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make sure to move typeObservers from old to new node when replacing nodes #1783

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

oxisto
Copy link
Member

@oxisto oxisto commented Oct 7, 2024

Previously, the old node still had all the type observers, which meant that the nodes registered as observers did not get any updates. Instead, we now unregister all nodes from the old one and register them as type observers for the new node. Maybe we need to make this configurable instead of the default? We definitely need it when we replace call nodes int cast/construct expressions - but we might not need it for other cases.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.86%. Comparing base (344d366) to head (994e4eb).
Report is 1 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
.../de/fraunhofer/aisec/cpg/helpers/SubgraphWalker.kt 81.45% <100.00%> (+0.45%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oxisto oxisto force-pushed the register-type-observer-after-replace branch from 49e0a11 to 7963422 Compare October 7, 2024 06:15
Copy link
Contributor

@maximiliankaul maximiliankaul left a comment

Choose a reason for hiding this comment

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

Looks good to me. Two points:

  • the function doc explicitly states, that only EOG edges are re-wired -> might be good to include the type observers there, too.
  • I cannot comment on your question in your PR description. I do not see a reason not to have them rewired but I might be missing something in the big picture.

@oxisto
Copy link
Member Author

oxisto commented Oct 7, 2024

Looks good to me. Two points:

  • the function doc explicitly states, that only EOG edges are re-wired -> might be good to include the type observers there, too.
  • I cannot comment on your question in your PR description. I do not see a reason not to have them rewired but I might be missing something in the big picture.

Good point, I added the documentation.

@oxisto oxisto enabled auto-merge (squash) October 7, 2024 09:17
@oxisto oxisto merged commit 8dad7d1 into main Oct 7, 2024
2 checks passed
@oxisto oxisto deleted the register-type-observer-after-replace branch October 7, 2024 09:20
maximiliankaul pushed a commit that referenced this pull request Oct 8, 2024
… nodes (#1783)

* Make sure to move `typeObservers` from old to new node when replacing nodes

* Added doc for typeobservers
KuechA added a commit that referenced this pull request Oct 18, 2024
* new node for RaiseStatement + NodeBuilder code

* don't add python stuff in this branch

* started work on EOG and DFG

* doc

* raise: fluent and first test

* fluent: patch @oxisto

* Start work on DFG test for raise

* rename raise -> throw

* more renaming raise -> throw

* copy & paste handleThrowOperator

* Update cpg-core/src/test/kotlin/de/fraunhofer/aisec/cpg/graph/edges/flows/DataflowTest.kt

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Rename `findSymbols` into `lookupSymbolByName` (#1772)

* Rename `findSymbols` into `lookupSymbolByName`

This PR renames `findSymbols` into `lookupSymbolByName` as a more appropriate name, because it lookups a symbol by its name.

Fixes #1767

* Update cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/ScopeManager.kt

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Added documentation

---------

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Update dependency rollup to v4.24.0 (#1774)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Added language trait `HasImplicitReceiver` (#1778)

Added language trait `HasImplicit Receiver`

This is needed in preparation for #1777 to better handle access to fields/members of a class when an implicit receiver is used.

* Cleanup of `SymbolResolver` (#1777)

* Fixed crash in `getCodeOfSubregion` (#1776)

* Add new function `lookupUniqueTypeSymbolByName` (#1781)

* Add new function `lookupUniqueTypeSymbolByName`

This adds two new functions `ScopeManager.lookupUniqueTypeSymbolByName` and `Reference.doesReferToType`. This harmonizes a lot of boilerplate code in type resolver, cxx extra pass and resolve ambuigity pass, which were all trying to achieve the same thing.

Fixes #1766

* Fixed issue with Go test, more robust handling of wrapped references

* Addressed code review

* Make sure to move `typeObservers` from old to new node when replacing nodes (#1783)

* Make sure to move `typeObservers` from old to new node when replacing nodes

* Added doc for typeobservers

* `implicit()` only triggers code/location update now if its not empty (#1784)

Otherwise, we override the code/location again.

* Added `:=` as simple operator in Python (#1785)

Named expressions in Python use `:=` as operator. Therefore we need to include it in the language definition. Otherwise, the `access` value of a reference will not be set correctly.

* code review

* code review

* code review

* rename cause to parentException

* doc

* ThrowStatement: add toString

* fix tests

* Merge EOG, add tests

* Documentation

* More doc of EOG handling

---------

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>
Co-authored-by: Christian Banse <christian.banse@aisec.fraunhofer.de>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Alexander Kuechler <alexander.kuechler@aisec.fraunhofer.de>
KuechA added a commit that referenced this pull request Oct 18, 2024
* new node for RaiseStatement + NodeBuilder code

* don't add python stuff in this branch

* started work on EOG and DFG

* doc

* raise: fluent and first test

* fluent: patch @oxisto

* Start work on DFG test for raise

* rename raise -> throw

* more renaming raise -> throw

* python: raise

* copy & paste handleThrowOperator

* Update cpg-core/src/test/kotlin/de/fraunhofer/aisec/cpg/graph/edges/flows/DataflowTest.kt

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Rename `findSymbols` into `lookupSymbolByName` (#1772)

* Rename `findSymbols` into `lookupSymbolByName`

This PR renames `findSymbols` into `lookupSymbolByName` as a more appropriate name, because it lookups a symbol by its name.

Fixes #1767

* Update cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/ScopeManager.kt

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Added documentation

---------

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>

* Update dependency rollup to v4.24.0 (#1774)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Added language trait `HasImplicitReceiver` (#1778)

Added language trait `HasImplicit Receiver`

This is needed in preparation for #1777 to better handle access to fields/members of a class when an implicit receiver is used.

* Cleanup of `SymbolResolver` (#1777)

* Fixed crash in `getCodeOfSubregion` (#1776)

* Add new function `lookupUniqueTypeSymbolByName` (#1781)

* Add new function `lookupUniqueTypeSymbolByName`

This adds two new functions `ScopeManager.lookupUniqueTypeSymbolByName` and `Reference.doesReferToType`. This harmonizes a lot of boilerplate code in type resolver, cxx extra pass and resolve ambuigity pass, which were all trying to achieve the same thing.

Fixes #1766

* Fixed issue with Go test, more robust handling of wrapped references

* Addressed code review

* Make sure to move `typeObservers` from old to new node when replacing nodes (#1783)

* Make sure to move `typeObservers` from old to new node when replacing nodes

* Added doc for typeobservers

* `implicit()` only triggers code/location update now if its not empty (#1784)

Otherwise, we override the code/location again.

* Added `:=` as simple operator in Python (#1785)

Named expressions in Python use `:=` as operator. Therefore we need to include it in the language definition. Otherwise, the `access` value of a reference will not be set correctly.

* code review

* code review

* code review

* rename cause to parentException

* doc

* ThrowStatement: add toString

* fix tests

* Enable raising the exception in the with statement

* Test

* Test++

---------

Co-authored-by: KuechA <31155350+KuechA@users.noreply.github.com>
Co-authored-by: Christian Banse <christian.banse@aisec.fraunhofer.de>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Alexander Kuechler <alexander.kuechler@aisec.fraunhofer.de>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants