-
Notifications
You must be signed in to change notification settings - Fork 93
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
SONARPY-2467 Fix performance bottleneck of GlobalSymbolComputation due to DjangoViewsVisitor #2246
Conversation
0bfbdc9
to
c57a8cd
Compare
…e to DjangoViewsVisitor
c57a8cd
to
16ab469
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the change looks good.
I had two questions though about the changes in the test code
@@ -521,7 +521,7 @@ void importedStubModules() { | |||
"""); | |||
ProjectLevelSymbolTable projectLevelSymbolTable = ProjectLevelSymbolTable.empty(); | |||
projectLevelSymbolTable.addModule(tree, "my_package", pythonFile("mod.py")); | |||
assertThat(projectLevelSymbolTable.typeShedDescriptorsProvider().stubModules()).containsExactlyInAnyOrder("math", "os"); | |||
assertThat(projectLevelSymbolTable.typeShedDescriptorsProvider().stubModules()).containsExactlyInAnyOrder("math", "os", "django", "django.urls.conf", "django.urls"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change comes from the fact that we always resolve those FQNs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We only used to do it when encountering a call expression (and on every call expression...)
Now we do it on file input. Here, the "file" only contains 2 imports, so it didn't resolve Django, but now it does it eagerly.
In practice we don't expect to find many files without call expressions anyway...
@@ -1113,7 +1113,7 @@ void write_cpd_tokens_to_cache() throws IOException { | |||
|
|||
assertThat(writeCache.getData().keySet()).containsExactlyInAnyOrder( | |||
"python:cache_version", "python:files", "python:descriptors:moduleKey:pass.py", "python:imports:moduleKey:pass.py", | |||
"python:cpd:data:moduleKey:pass.py", "python:cpd:stringTable:moduleKey:pass.py", "python:content_hashes:moduleKey:pass.py"); | |||
"python:cpd:data:moduleKey:pass.py", "python:cpd:stringTable:moduleKey:pass.py", "python:content_hashes:moduleKey:pass.py", "python:typeshed_modules"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why this change is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because now every project will look into Typeshed for at least Django.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thx for the answers
SONARPY-2467
This change avoids recreating the same
TypeCheckBuilder
object repeatedly when performing the Django views visit. Instead, it uses only one for each file.This implies that we're going to load Typeshed stubs for Django on all files, even if they don't contain
CalleExpression
. While this implication should have no real-world impact, this is what changes some of the project level symbol table and sensor tests