-
Notifications
You must be signed in to change notification settings - Fork 85
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-1815 Enable AST-based type inference for functions/module containing try/catch blocks #1792
Conversation
e60a8be
to
7c38c22
Compare
workList.push(expression); | ||
while (!workList.isEmpty()) { | ||
Expression e = workList.pop(); | ||
if (e.is(Tree.Kind.NAME)) { |
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 is possible to do type check and type cast in single operation here:
if (e instanceof Name name) {
...
}
|
||
public record Assignment(SymbolV2 lhsSymbol, Name lhsName, Expression rhs) { | ||
public class Assignment { |
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.
do we really need to have it as a class instead of a record?
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 don't have a strong opinion on what it should be, but if you don't mind, I'd rather keep this as it is here, since SONARPY-1826 refactors this again. Please have a look at that PR and I'm happy to further refactor it there into a record if that still makes sense (I didn't consider this tbh).
final SymbolV2 lhsSymbol; | ||
Name lhsName; | ||
Expression rhs; | ||
Set<SymbolV2> variableDependencies = new HashSet<>(); |
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'd prefer to have a single place to initialize everything. If you have a constructor - may be it makes sense to initialize these fields there as well
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.
If you don't mind I'll address this as part of SONARPY-1826.
void computeDependencies(Expression expression, Set<SymbolV2> trackedVars) { | ||
Deque<Expression> workList = new ArrayDeque<>(); | ||
workList.push(expression); | ||
while (!workList.isEmpty()) { |
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.
Add a comment here to rework this part later. Add tests showing type tracking of collection item types
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 didn't add the comment, as I think we already have some tickets that relate to this anyway. However, I added a test that shows that collection item types actually work (for a trivial case).
Set<Assignment> workSet = new HashSet<>(propagations); | ||
while (!workSet.isEmpty()) { | ||
Iterator<Assignment> iterator = workSet.iterator(); | ||
Assignment propagation = iterator.next(); |
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.
rename to assignment
and propagations
parameter as well
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.
This will be updated in SONARPY-1826 (introducing Propagation
class again).
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, just couple of small changes requested
7c38c22
to
d48bdfa
Compare
27fde47
to
245a7f4
Compare
Quality Gate failedFailed conditions See analysis details on SonarQube Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
…taining try/catch blocks
ffc227a
to
e95d4d2
Compare
081dc4a
into
MMF-3796
…taining try/catch blocks (#1792)
…taining try/catch blocks (#1792)
No description provided.