Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Do not resolve unqualified symbols in current record without HasImplicitReceiver #1984

Merged
merged 12 commits into from
Jan 29, 2025
Merged

Do not resolve unqualified symbols in current record without HasImplicitReceiver #1984

merged 12 commits into from
Jan 29, 2025

Conversation

oxisto
Copy link
Member

@oxisto oxisto commented Jan 25, 2025 •
edited
Loading

We need to consider the trait HasImplicitReceiver (or the lack thereof) when resolving symbols in an unqualified lookup to not accidentally resolve fields/methods of a class without an receiver.

Fixes #1983

@codecov Codecov
Copy link

codecov bot commented Jan 25, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.64516% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.03%. Comparing base (167a54d) to head (724bee6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...kotlin/de/fraunhofer/aisec/cpg/graph/Extensions.kt 66.66% 1 Missing and 1 partial ⚠️
...s/kotlin/de/fraunhofer/aisec/cpg/test/TestUtils.kt 33.33% 1 Missing and 1 partial ⚠️
...tlin/de/fraunhofer/aisec/cpg/graph/scopes/Scope.kt 80.00% 0 Missing and 1 partial ⚠️
.../de/fraunhofer/aisec/cpg/frontends/TestLanguage.kt 50.00% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
...ain/kotlin/de/fraunhofer/aisec/cpg/ScopeManager.kt 82.38% <100.00%> (+0.28%) ⬆️
...e/fraunhofer/aisec/cpg/frontends/LanguageTraits.kt 100.00% <ø> (ø)
...fer/aisec/cpg/frontends/python/StatementHandler.kt 84.11% <100.00%> (+0.05%) ⬆️
...ofer/aisec/cpg/passes/PythonAddDeclarationsPass.kt 90.66% <100.00%> (+0.25%) ⬆️
...tlin/de/fraunhofer/aisec/cpg/graph/scopes/Scope.kt 81.69% <80.00%> (+0.80%) ⬆️
.../de/fraunhofer/aisec/cpg/frontends/TestLanguage.kt 80.55% <50.00%> (-2.31%) ⬇️
...kotlin/de/fraunhofer/aisec/cpg/graph/Extensions.kt 62.53% <66.66%> (-0.19%) ⬇️
...s/kotlin/de/fraunhofer/aisec/cpg/test/TestUtils.kt 72.82% <33.33%> (-1.34%) ⬇️

... and 6 files with indirect coverage changes

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

@oxisto oxisto changed the title Introduce HasExplicitMemberAccess trait Introduce HasExplicitMemberAccessOnly trait Jan 25, 2025
@oxisto oxisto changed the title Introduce HasExplicitMemberAccessOnly trait Introduce HasExplicitReceiverOnly trait Jan 25, 2025
@oxisto oxisto mentioned this pull request Jan 25, 2025
Closed
@oxisto oxisto force-pushed the fix-python-unqualified-lookup branch from 5452d7a to 3488de7 Compare January 28, 2025 09:04
@oxisto
Copy link
Member Author

oxisto commented Jan 28, 2025

After some digging, I actually discovered that we already have the opposite trait HasImplicitReceiver, so we should probably use the existing one instead.

@oxisto oxisto marked this pull request as draft January 28, 2025 21:33
@oxisto oxisto changed the title Introduce HasExplicitReceiverOnly trait Do not resolve symbols in current record without HasImplicitReceiver Jan 28, 2025
@oxisto oxisto marked this pull request as ready for review January 28, 2025 22:08
@oxisto oxisto force-pushed the fix-python-unqualified-lookup branch 2 times, most recently from 66cd738 to 706df94 Compare January 28, 2025 22:23
@oxisto oxisto changed the title Do not resolve symbols in current record without HasImplicitReceiver Do not resolve unqualified symbols in current record without HasImplicitReceiver Jan 29, 2025
maximiliankaul
maximiliankaul requested changes Jan 29, 2025
View reviewed changes
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.

In general, I'm happy with this PR, once the additional case of fields declared directly inside a class are covered.

...nguage-python/src/test/kotlin/de/fraunhofer/aisec/cpg/frontends/python/SymbolResolverTest.kt Show resolved Hide resolved
...nguage-python/src/test/kotlin/de/fraunhofer/aisec/cpg/frontends/python/SymbolResolverTest.kt Outdated Show resolved Hide resolved
...nguage-python/src/test/kotlin/de/fraunhofer/aisec/cpg/frontends/python/SymbolResolverTest.kt Show resolved Hide resolved
@oxisto oxisto force-pushed the fix-python-unqualified-lookup branch from 04ef9cf to 5bdffd9 Compare January 29, 2025 12:57
maximiliankaul
maximiliankaul approved these changes Jan 29, 2025
View reviewed changes
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.

LGTM :)

cpg-language-python/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/PythonAddDeclarationsPass.kt Outdated Show resolved Hide resolved
cpg-language-python/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/PythonAddDeclarationsPass.kt Outdated Show resolved Hide resolved
@oxisto oxisto force-pushed the fix-python-unqualified-lookup branch from 9404be7 to 724bee6 Compare January 29, 2025 17:09
@oxisto oxisto enabled auto-merge (squash) January 29, 2025 17:09
@oxisto oxisto merged commit c72becd into main Jan 29, 2025
2 checks passed
@oxisto oxisto deleted the fix-python-unqualified-lookup branch January 29, 2025 17:15
oxisto added a commit that referenced this pull request Jan 29, 2025
@oxisto @maximiliankaul
Do not resolve unqualified symbols in current record without `HasImpl…
48a43cb 
…icitReceiver` (#1984)

* Introduce `HasExplicitMemberAccess` trait

Fixes #1983

* Added trait to GoLanguage as well to be safe

* Added more documentation

* Spotless

* Addressed code review

* Using existing trait

* added more tests

* Fixed the tests as good as they can be fixed in this PR

* a -> globalA

* Fixed another bug where field declarations were declared twice

* Update cpg-language-python/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/PythonAddDeclarationsPass.kt

Co-authored-by: Maximilian Kaul <[email protected]>

* Better documentation

---------

Co-authored-by: Maximilian Kaul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KuechA KuechA KuechA approved these changes

@maximiliankaul maximiliankaul maximiliankaul approved these changes

@lshala lshala Awaiting requested review from lshala lshala is a code owner

@konradweiss konradweiss Awaiting requested review from konradweiss konradweiss is a code owner

Assignees
No one assigned
Labels
go python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python incorrectly resolves fields in unqualified lookup
3 participants
@oxisto @maximiliankaul @KuechA