SONARPY-2336 Ensure ClassDescriptor and FunctionDescriptor fullyQualifiedName are not nullable #2157
Conversation
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2336
branch
8 times, most recently
from
50375af to
8beb3a7
Compare
guillaume-dequenne-sonarsource
changed the title
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2336
branch
from
7e411f8 to
f378835
Compare
guillaume-dequenne-sonarsource
requested a review
from thomas-serre-sonarsource
There was a problem hiding this comment.
LGTM, great work 💪 !
I left many minor comments, and I think it is worth it to have a second round of review.
On the JIRA side, it looks a bit unclear to me. In my opinion you are tackling 3 tickets in this PR :
- SONARPY-2336 as written in the PR title
- SONARPY-2339 as commented in the PR
- SONARPY-???? which does not exist and is meant to convert ***TypeTest to V2 (see comments in the review for a clearer explanation).
I'm okay with tackling several tickets in the same PR, but I'm unsure if the jira automation will work in this case, and I would prefer to see the 3 jira tickets in the title of the PR. It could be worth it to settle this so the development process covers this case.
python-frontend/src/main/java/org/sonar/python/index/DescriptorsToProtobuf.java
Outdated
Show resolved
Hide resolved
python-frontend/src/main/java/org/sonar/python/semantic/v2/ClassTypeBuilder.java
Outdated
Show resolved
Hide resolved
python-frontend/src/main/java/org/sonar/python/semantic/v2/ClassTypeBuilder.java
Outdated
Show resolved
Hide resolved
...n-frontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java
Show resolved
Hide resolved
| private AmbiguousDescriptor lastAmbiguousDescriptor(String... code) { | ||
| Symbol ambiguousSymbol = lastSymbolFromDef(code); | ||
| if (!(ambiguousSymbol instanceof AmbiguousSymbol)) { | ||
| Name name = lastName(code); | ||
| PythonType pythonType = name.typeV2(); | ||
| if (!(pythonType instanceof UnionType unionType)) { | ||
| throw new AssertionError("Symbol is not ambiguous."); | ||
| } | ||
| AmbiguousDescriptor ambiguousDescriptor = (AmbiguousDescriptor) descriptor(ambiguousSymbol); | ||
| assertThat(ambiguousDescriptor.name()).isEqualTo(ambiguousSymbol.name()); | ||
| assertThat(ambiguousDescriptor.fullyQualifiedName()).isEqualTo(ambiguousSymbol.fullyQualifiedName()); | ||
| SymbolV2 symbol = name.symbolV2(); | ||
| PythonTypeToDescriptorConverter converter = new PythonTypeToDescriptorConverter(); | ||
| AmbiguousDescriptor ambiguousDescriptor = (AmbiguousDescriptor) converter.convert("my_package.mod", symbol, Set.of(unionType)); | ||
| assertThat(ambiguousDescriptor.name()).isEqualTo(name.name()); | ||
| assertThat(ambiguousDescriptor.fullyQualifiedName()).isEqualTo("my_package.mod." + symbol.name()); | ||
| return ambiguousDescriptor; | ||
| } |
There was a problem hiding this comment.
Oh okay, you have converted those tests to V2! It took me quite long to understand why those tests have been impacted by your changes :-)
I think this is outside the scope of the current ticket. I suggest either updating the ticket description to note that you did this also in this PR or creating another ticket that you will link to this PR.
Usually, I would prefer to have different PRs/tickets so it is easier to review but since you have it already there, I would suggest to keep it like this!
...nd/src/main/java/org/sonar/python/semantic/v2/converter/PythonTypeToDescriptorConverter.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void multipleClassSymbolNotAmbiguous() { | ||
| ClassDescriptor classDescriptor = lastClassDescriptor( | ||
| """ | ||
| class A: ... | ||
| class A: ... | ||
| """ | ||
| ); | ||
| assertThat(classDescriptor.name()).isEqualTo("A"); | ||
| assertThat(classDescriptor.fullyQualifiedName()).isEqualTo("my_package.mod.A"); | ||
| assertDescriptorToProtobuf(classDescriptor); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice that you moved this test here 👍
python-frontend/src/test/java/org/sonar/python/index/FunctionDescriptorTest.java
Outdated
Show resolved
Hide resolved
I think the 3rd ticket you're referring to matches SONARPY-2346. I'm tackling a small part of it here because it's needed to repair the otherwise broken tests on When dealing with multiple tickets in a single PR, I personally don't like having them all in the PR title (I tend to think it feels bloated then). I tend to have the "main" ticket as a PR title, and the prerequisite as a separate commit. Indeed I don't think Jira automation works in this case. We can probably revisit this way of doing so that it works better with automation though. |
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2336
branch
from
10e0d5f to
fabce92
Compare
guillaume-dequenne-sonarsource
requested a review
from thomas-serre-sonarsource
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2336
branch
from
fabce92 to
05a1afb
Compare
|
SONARPY-2339
Following this PR, there are possible null arguments to
ClassDescriptor#fullQualifiedNameandFunctionDescriptor#fullQualifiedNameinDescriptorUtils. These methods are no longer used in production code, and are only used in tests (mostly inProjectLevelSymbolTableTestthroughProjectLevelSymbolTable#from). SONARPY-2346 has been created to handle this.