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.

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

Python: Resolve import types for superclasses #2034

Merged
merged 9 commits into from
Feb 11, 2025
Merged

Conversation

lshala
Copy link
Contributor

@lshala lshala commented Feb 7, 2025

This PR adds the handling for nested AST.Attributes, ensuring that superclass imports are correctly resolved

@lshala lshala added the python label Feb 7, 2025
@lshala lshala changed the title Resolve import types for superclasses Python: Resolve import types for superclasses Feb 7, 2025
@lshala lshala marked this pull request as ready for review February 7, 2025 10:44
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.26%. Comparing base (e53b568) to head (7c8dd00).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...sec/cpg/frontends/python/PythonLanguageFrontend.kt 66.66% 1 Missing and 4 partials ⚠️

❌ Your patch status has failed because the patch coverage (72.22%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
Files with missing lines Coverage Δ
...ain/kotlin/de/fraunhofer/aisec/cpg/ScopeManager.kt 83.28% <100.00%> (ø)
...sec/cpg/frontends/python/PythonLanguageFrontend.kt 80.50% <66.66%> (-0.42%) ⬇️

... and 2 files with indirect coverage changes

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

@lshala lshala added the publish-to-github-packages If added to a PR, builds from it will be published as a GitHub package label Feb 7, 2025
@lshala lshala marked this pull request as draft February 7, 2025 16:27
@lshala
Copy link
Contributor Author

lshala commented Feb 10, 2025

Unfortunately, this change is causing errors in the SymbolResolver. I tried debugging and noticed that the getInvocationCandidatesFromParents method is running in an endless loop. Need your help @oxisto

@lshala lshala force-pushed the ls/resolve-import-types branch from 70e18d4 to 6fe6aff Compare February 10, 2025 09:59
@oxisto
Copy link
Member

oxisto commented Feb 10, 2025

Unfortunately, this change is causing errors in the SymbolResolver. I tried debugging and noticed that the getInvocationCandidatesFromParents method is running in an endless loop. Need your help @oxisto

Where exactly? Do you have a test file?

@lshala
Copy link
Contributor Author

lshala commented Feb 10, 2025

Unfortunately, this change is causing errors in the SymbolResolver. I tried debugging and noticed that the getInvocationCandidatesFromParents method is running in an endless loop. Need your help @oxisto

Where exactly? Do you have a test file?

I tried to parse the cinder/api folder and the error occurs here: urlmap.py

@oxisto
Copy link
Member

oxisto commented Feb 10, 2025

Unfortunately, this change is causing errors in the SymbolResolver. I tried debugging and noticed that the getInvocationCandidatesFromParents method is running in an endless loop. Need your help @oxisto

Where exactly? Do you have a test file?

I tried to parse the cinder/api folder and the error occurs here: urlmap.py

There seems to be a confusion in https://github.com/openstack/cinder/blob/a1fb7da213b72040b8c3fa2b2708f23c21da20b2/cinder/api/urlmap.py#L168 the super declaration is the same as the declaration after resolving. Which is weird because the record is cinder.api.urlmap.URLMap and the super type is supposed to be paste.urlmap.URLMap, so either the parsed name is wrong, or somehow it gets resolved incorrectly.

@oxisto
Copy link
Member

oxisto commented Feb 10, 2025

A good fix for that seems to be (partially) blocked by #2020

@oxisto oxisto force-pushed the ls/resolve-import-types branch from 6b25f96 to f667a0d Compare February 10, 2025 14:14
@oxisto oxisto changed the base branch from main to revamped-imported-symbols February 10, 2025 14:15
@oxisto oxisto force-pushed the ls/resolve-import-types branch from f667a0d to 71656ee Compare February 10, 2025 14:15
@oxisto oxisto force-pushed the revamped-imported-symbols branch from 8380b24 to e588a24 Compare February 10, 2025 14:16
@oxisto oxisto force-pushed the ls/resolve-import-types branch from 71656ee to db7d529 Compare February 10, 2025 14:16
@oxisto oxisto force-pushed the revamped-imported-symbols branch from 03a114d to 1de14ed Compare February 10, 2025 15:24
Base automatically changed from revamped-imported-symbols to main February 10, 2025 15:28
@oxisto oxisto force-pushed the ls/resolve-import-types branch from db7d529 to 7dd8790 Compare February 10, 2025 15:55
@lshala lshala marked this pull request as ready for review February 11, 2025 08:21
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

@oxisto oxisto force-pushed the ls/resolve-import-types branch from 982872f to 7c8dd00 Compare February 11, 2025 12:30
@oxisto oxisto merged commit 25c22cd into main Feb 11, 2025
3 of 4 checks passed
@oxisto oxisto deleted the ls/resolve-import-types branch February 11, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
publish-to-github-packages If added to a PR, builds from it will be published as a GitHub package python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants