Skip to content

Conversation

topolik
Copy link
Contributor

@topolik topolik commented Oct 14, 2024

Looks like the problem is there from the very beginnings where the logic ignored all non-parametric methods as "not interesting".

Omitting those methods from the MultiMap doesn't give TopologySort any information about the graph orientation and these methods are randomly misplaced when calling ClassContext#getMethodsInCallOrder.

Part of the PR is a test that has a simple call stack from the default static{} constructor <clinit>()V through several static methods to a non-parametric class constructor, one more constructor and then instantiated class methods which creates a very simple DAG that any topological sorting algorithm should have no problem analyzing.

After fixing the interestingSignature method from return !"()V".equals(signature); to return true;, it works! :)

FYI: required for proper FindSecurityBugs taint analysis of derived methods. Without proper order they are skipped by the analysis engine as unknown.

@JuditKnoll JuditKnoll linked an issue Oct 15, 2024 that may be closed by this pull request
Copy link
Collaborator

@JuditKnoll JuditKnoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!
Can you please add one more test, where one of the functions called multiple times in a way that it could have affect on the order?

Can you please add an entry to the CHANGELOG.md file's unreleased part?

@topolik
Copy link
Contributor Author

topolik commented Oct 17, 2024

Can you please add one more test, where one of the functions called multiple times in a way that it could have affect on the order?

Hi @JuditKnoll , just to confirm my understanding, I can think of 2 different kinds that impact the order.

First case is when A -> C and B -> C. In such scenario we call C twice. C goes first and then A and B go next as second together, so we don't care about the order of A and B.

Another case is A -> C and A -> B -> C. In this scenario C goes first, then B and then A. We call C twice but the order in the call graph is fixed.

Thank you

@JuditKnoll
Copy link
Collaborator

Can you please add one more test, where one of the functions called multiple times in a way that it could have affect on the order?

Hi @JuditKnoll , just to confirm my understanding, I can think of 2 different kinds that impact the order.

First case is when A -> C and B -> C. In such scenario we call C twice. C goes first and then A and B go next as second together, so we don't care about the order of A and B.

Another case is A -> C and A -> B -> C. In this scenario C goes first, then B and then A. We call C twice but the order in the call graph is fixed.

Thank you

I was thinking about the second scenario.

@topolik
Copy link
Contributor Author

topolik commented Oct 18, 2024

Hi @JuditKnoll,

please see the new commits.

Thanks.

Copy link
Collaborator

@JuditKnoll JuditKnoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add an entry to the CHANGELOG.md file's unreleased part?

@topolik
Copy link
Contributor Author

topolik commented Oct 25, 2024

Ops, forgot. Thank you @JuditKnoll.

@topolik
Copy link
Contributor Author

topolik commented Oct 25, 2024

There was a conflict in CHANGELOG.md. Rebased.

@hazendaz hazendaz added this to the SpotBugs 4.9.0 milestone Oct 31, 2024
@JuditKnoll JuditKnoll merged commit 38c88f9 into spotbugs:master Oct 31, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect method call order
4 participants