Change ValueRangeAnalysis to a data-flow analysis #2073

baloghadamsoftware · 2022-05-27T16:26:55Z

Currently, SpotBugs includes a value-range analysis which is only used
for detecting redundant conditions. However, it could be used for
detecting other kind of errors, such as arithmetic errors as well. This
PR refactors this analysis to a data-flow based one while keeping the
functionality of the existing redundant condition analysis based on top
of it.

Make sure these boxes are checked before submitting your PR -- thank you!

Added an entry into CHANGELOG.md if you have changed SpotBugs code

Currently, SpotBugs includes a value-range analysis which is only used for detecting redundant conditions. However, it could be used for detecting other kind of errors, such as arithmetic errors as well. This PR refactors this analysis to a data-flow based one while keeping the functionality of the existing redundant condition analysis based on top of it.

baloghadamsoftware · 2022-06-02T11:48:58Z

An example for the usefulness of this PR is finding arithmetic errors such as division by zero: baloghadamsoftware#3

baloghadamsoftware · 2022-06-08T13:20:28Z

I tested this on large projects. No crashes and no speed drop.

baloghadamsoftware · 2022-06-08T14:00:06Z

I found a true positive (redundant condition) in an open-source project using this new approach: True positive in MATSim-Libs

iloveeclipse

I have no time to do a real review, here just a formal one. In general, with enough test coverage, I think it makes sense.

iloveeclipse · 2022-06-08T14:54:26Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/LongRangeSet.java

+    private static final Map<String, TypeLongRange> typeRanges;
+
+    static {
+        typeRanges = new HashMap<>();


Should it be Collections.unmodifiableMap()?
Also I would expect it to be UPPER_CASE as it is a constant map.

I agree. This is also from the old code, but now now this whole map is constant so I made it unmodifiable.

iloveeclipse · 2022-06-08T14:55:36Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/LongRangeSet.java

+        return typeRanges.containsKey(signature);
+    }
+
+    private SortedMap<Long, Long> map = new ConcurrentSkipListMap<>();


Please give some meaningful name, like rangesMap and explanation what is the key and what is the value.

Is there a good reason to use concurrent map here? Do we really expect the set to be modified by multiple threads? If so, class javadoc should explain that this is supposed to be thread safe data structure.

No, this was a misunderstanding. I got several ConcurrentModificationExceptions but they were because I tried to merge the same instance into itself. A check was missing to prevent this.

iloveeclipse · 2022-06-08T14:58:29Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/TypeLongRange.java

+import java.util.Set;
+
+public class TypeLongRange {
+    long min, max;


Should the fields be final? If not, something should be done to avoid unintentional change of values for cases where they are used as constant values (like in LongRangeSet.typeRanges)

Good point!

iloveeclipse · 2022-06-08T14:59:01Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/Branch.java

@@ -0,0 +1,65 @@
+package edu.umd.cs.findbugs.ba;


Here and in all other classes, please add license headers.

All the classes got license headers now, except the VariableData class and the ones in the spotbugs-tests and spotbugsTestCases directories.

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/Branch.java

iloveeclipse · 2022-06-08T15:11:31Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/LongRangeSet.java

+
+    private void add(Long start, Long end) {
+        if (map.isEmpty()) {
+            map.put(start, end);


this "if" block is not thread safe, and two threads can enter the condition at same time. If this type is supposed to be thread safe, this is a bug.

Actually entire method is not thread safe, in almost all cases below.

See my comment above: this class does not need to be thread safe.

iloveeclipse · 2022-06-08T15:14:50Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ValueRangeMap.java

+        branch = null;
+    }
+
+    public void meetWith(ValueRangeMap other) {


meetWith is unusual. Did you mean mergeWith ?

I saw meetWith() in LockSet which is the fact for LockAnalysis. Thus since ValueRangeMap is the fact for ValueRangeAnalysis I used the same name. However, I can change it if you think mergeWith is a better name.

iloveeclipse · 2022-06-08T15:21:04Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FieldItemSummary.java

@@ -116,7 +116,7 @@ public void sawOpcode(int seen) {
                        fieldSummary.addWrittenOutsideOfConstructor(fieldOperand);
                    }
                }
-            } else if (seen == Const.PUTSTATIC && !Const.STATIC_INITIALIZER_NAME.equals(getMethodName())) {
+            } else if (!Const.STATIC_INITIALIZER_NAME.equals(getMethodName())) {


Is this from unrelated fix? Please don't mix multiple fixes in one PR

Not entirely. For some reasons RedundantConditions can now find more true positives. This is one of them. If I do not fix it then the build fails because of SpotBugs errors.

iloveeclipse · 2022-06-08T15:21:09Z

...bugs/src/main/java/edu/umd/cs/findbugs/detect/FunctionsThatMightBeMistakenForProcedures.java

@@ -170,7 +170,7 @@ public void visit(Code code) {
            if (returnUnknown > 0) {
                priority++;
            }
-            if (returnNew > 0 && priority > NORMAL_PRIORITY) {
+            if (priority > NORMAL_PRIORITY) {


Is this from unrelated fix? Please don't mix multiple fixes in one PR

This is the other true positive.

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ValueRangeAnalysis.java

…hods & fields renamed

baloghadamsoftware · 2022-06-10T13:40:24Z

Thank you for your comments, @iloveeclipse! They were really useful and I did agree with all of them. Now I am considering adding test cases but I could not find any test case for analysises yet. We would need some kind of greybox testing here. Some framework where we can check facts of different data flows at some given points of the CFG. Do you know about one?

iloveeclipse · 2022-06-10T15:31:30Z

Do you know about one?

Not really, I would have to search in the code.

Note: I will be mostly offline for a week from tomorrow, so don't expect any feedback from me in that time.

baloghadamsoftware · 2022-06-10T19:08:13Z

Do you know about one?

Not really, I would have to search in the code.

Note: I will be mostly offline for a week from tomorrow, so don't expect any feedback from me in that time.

No problem, you already helped a lot. I think I found something that I could maybe use for testing.

baloghadamsoftware · 2022-09-09T13:40:06Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ValueRangeAnalysis.java

+    }
+
+    @Override
+    public void meetInto(ValueRangeMap fact, Edge edge, ValueRangeMap result) throws DataflowAnalysisException {


Maybe I should split the logic of this method to edgeTransfer() and meetInto()?

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/ValueRangeDataflowTest.java

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ValueRangeMap.java

baloghadamsoftware · 2022-09-28T10:11:11Z

@KengoTODA Could you take a look on this one, please? This PR could be a huge improvement which could pave the way to more complex detectors based on the actual possible values of the variables.

baloghadamsoftware · 2023-03-01T15:39:12Z

@KengoTODA? and/or @iloveeclipse?

JuditKnoll

The primitive type signatures are used in more classes (LongRangeSet, ValueRangeAnalysis, RedundantConditions) more times, wouldn't it be worth to use constants instead of the hardcoded signatures?
I think this feature seems really promising, and could improve the capabilities of Spotbugs in a great sense.

JuditKnoll · 2023-03-20T12:19:46Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/Branch.java

+
+
+/**
+ * This class describes an edge in the CFG which belongs to a branch statemen: IFCMP edges and the fallback edges


There is a missing 't' from the end of the word "statement".

JuditKnoll · 2023-03-20T14:39:24Z

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ValueRangeAnalysis.java

+import edu.umd.cs.findbugs.classfile.Global;
+import edu.umd.cs.findbugs.classfile.MethodDescriptor;
+
+public class ValueRangeAnalysis extends ForwardDataflowAnalysis<ValueRangeMap> {


It would be really nice, if this class had a short javadoc explaining the purpose of the class. However, I'm not sure, what exactly decides whether a class needs javadoc or the licence header.

JuditKnoll · 2023-03-20T15:27:11Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RedundantConditions.java

@@ -193,7 +398,7 @@ private int getPriority(MethodDescriptor methodDescriptor, RedundantCondition co
        return priority;
    }

-    /**
+    /*


Was the deletion of this '*' intentional?

Ádám Balogh added 2 commits May 27, 2022 18:23

Merge branch 'master' into ValueRangeDataflow

0fe87ac

Optimize LongRangeSet to reduce unneccessary object creations

96360b7

baloghadamsoftware marked this pull request as ready for review June 8, 2022 14:01

iloveeclipse requested changes Jun 8, 2022

View reviewed changes

License headers and JavaDoc comments added, unused stuff removed, met…

0bc9707

…hods & fields renamed

Ádám Balogh added 2 commits June 15, 2022 12:14

Add tests to ValueRangeDataflow

b83db64

Tests for LongRangeSet added

918abe5

baloghadamsoftware mentioned this pull request Sep 2, 2022

False negative about the rule SQL_BAD_RESULTSET_ACCESS in java #2137

Open

baloghadamsoftware requested a review from iloveeclipse September 2, 2022 09:53

baloghadamsoftware commented Sep 9, 2022

View reviewed changes

ThrawnCA requested changes Sep 12, 2022

View reviewed changes

Updated according to the comments of @ThrawnCA

ae3d629

ThrawnCA approved these changes Sep 15, 2022

View reviewed changes

JuditKnoll reviewed Mar 20, 2023

View reviewed changes

github-actions bot added the Stale label Mar 4, 2024

github-actions bot closed this Apr 8, 2024

hazendaz reopened this Apr 12, 2024

github-actions bot removed the Stale label Apr 13, 2024

JuditKnoll mentioned this pull request Apr 15, 2024

False Positive: IL_INFINITE_LOOP #2936

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change ValueRangeAnalysis to a data-flow analysis #2073

Change ValueRangeAnalysis to a data-flow analysis #2073

baloghadamsoftware commented May 27, 2022

baloghadamsoftware commented Jun 2, 2022

baloghadamsoftware commented Jun 8, 2022

baloghadamsoftware commented Jun 8, 2022

iloveeclipse left a comment

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 9, 2022

iloveeclipse Jun 8, 2022

JuditKnoll Mar 31, 2023

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

iloveeclipse Jun 8, 2022

baloghadamsoftware Jun 10, 2022

baloghadamsoftware commented Jun 10, 2022

iloveeclipse commented Jun 10, 2022

baloghadamsoftware commented Jun 10, 2022

baloghadamsoftware Sep 9, 2022

baloghadamsoftware commented Sep 28, 2022

baloghadamsoftware commented Mar 1, 2023 •

edited

JuditKnoll left a comment

JuditKnoll Mar 20, 2023

JuditKnoll Mar 20, 2023

JuditKnoll Mar 20, 2023



		/**
		* This class describes an edge in the CFG which belongs to a branch statemen: IFCMP edges and the fallback edges

Change ValueRangeAnalysis to a data-flow analysis #2073

Are you sure you want to change the base?

Change ValueRangeAnalysis to a data-flow analysis #2073

Conversation

baloghadamsoftware commented May 27, 2022

baloghadamsoftware commented Jun 2, 2022

baloghadamsoftware commented Jun 8, 2022

baloghadamsoftware commented Jun 8, 2022

iloveeclipse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloghadamsoftware commented Jun 10, 2022

iloveeclipse commented Jun 10, 2022

baloghadamsoftware commented Jun 10, 2022

Choose a reason for hiding this comment

baloghadamsoftware commented Sep 28, 2022

baloghadamsoftware commented Mar 1, 2023 • edited

JuditKnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloghadamsoftware commented Mar 1, 2023 •

edited