Report all ref comparisons: use property to report all ref comparisons when set #2988
Conversation
|
@JuditKnoll thank you, I've added a couple of simple tests, you will notice I had to make a change in order to make this testeable. I'm not familiar with bcel, but is there a way to identify if a |
|
Am I correct in reading this that it was always 'true' before this change for reportAll, now it will be false by default? I think we should at least adhere to the current default if that is the case. |
|
@hazendaz it was true but it was not reading the system property. I think the Edit: I removed the clause at 66e6659 just to showcase that the test fails without it |
|
@JuditKnoll @hazendaz let me know if this looks good or if there's anything to address |
|
hi, any update on when this can be merged and released? |
|
@miguel-vila Hi, my concern is that line 890 expects the current default of 'true'. Since now it expects a system variable, I'm presuming that variable is not there by default ? If it is not there by default, that breaks how this works today and thus would need to wait until we move to version 5. If you want this in the next release which I think probably will happen this weekend, I'd suggest leaving the default 'true' as it is today instead of now 'false'. That assumes my assumption on this is accurate but just from a code perspective it seems to be a breaking change. |
…s into report-all-ref-comps
|
@hazendaz hmmm, I assume your concern is that this doesn't have the same behaviour as the previous code? So in order to preserve the default behaviour before (around (btw I don't think I understand the Is there a series branch for 5, or how do you keep the code targetting that version? When are you expecting to release that version? |
|
@miguel-vila I'm good now with that. I'm unsure how the original code works but we have had so many mishaps lately that we should try our best not to break the original expectations. Main thing there was that it had that check expecting true there so if not present that was the original processing. I'm completely ok with your new processing otherwise. I just wanted to error on side of caution since the first path would have changed. Now it should remain the same. We don't have a specific 5.x branch yet. Since we have had a number of issues in recent releases, we have been trying to stabilize before we jump as we will be changing the java version. Unfortunately we don't have a lot of active contributors at the moment so moving to 5.x has been slower than expected. If @JuditKnoll, @ThrawnCA, and @gtoison all are ok on this, I'm ok with it coming in for 4.8.6. I think unless anyone has something else that we cut this weekend as it has a couple of fixes otherwise over last release. I know there are still some issues so 4.8.x might drag on a while longer in general but trying to get what works out the door in general. |
|
@miguel-vila Can you check the new failures? |
|
looked into this. By making the variable have a default value of true, this error surfaced: it was trying to instantiate Other than that, after enabling this it turns out spotbugs codebase itself has 34 cases of ref comparisons, so:
|
|
oh no, it's not 34 cases, it's like 397 😬 |
|
@gtoison I have made some changes: the new |
|
@gtoison if we want to use this, would we need to set the priority to |
|
Good question! I think one way is to set the priority to low and then add a filter to exclude all low priority bugs, except for that one |
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindRefComparisonTest.java
Outdated
Show resolved
Hide resolved
| public static void removeProperty(String name) { | ||
| properties.remove(name); | ||
| } |
There was a problem hiding this comment.
I'm not really fond of adding a new function just for the sake of tests. However, I don't know how else could this be tested without significant structural modification.
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindRefComparison.java
Outdated
Show resolved
Hide resolved
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindRefComparisonTest.java
Outdated
Show resolved
Hide resolved
| private void assertRefCompBug(String method, int line, Confidence confidence) { | ||
| final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder() | ||
| .bugType("RC_REF_COMPARISON") | ||
| .inClass("RefComparisons") |
There was a problem hiding this comment.
Are you sure that it's in this class every time? Even when it's in the inner class RefComparisons$MyClass3?
There was a problem hiding this comment.
hmm, I added a class parameter to this method to make this more explicitly, but I think it should be OK. I first check the total number of bugs (no matter the class) and then I use this method to check for each one of them.
|
hi, any update on whether this can be merged as it is, or should I address anything? |
|
thanks for merging, when is the next planned release? |
|
possibly next week. no real timetable but that might work. |
|
@hazendaz any update on when the next release is going to be? I see this PR was added to the 4.9.0 milestone |
|
Hi, not entirely sure. I've meant to get to it but haven't found time yet. Possibly one of the other members might pick it up for release. I believe we have to discuss first though as I believe we skipped commons-lang update a few releases so at very minimum the changes need to be reviewed before we but it. That could easily take any one of us a day to review. Will try to get it soon though as I though I could get it last month and still haven't. Sorry for delays. |
|
@hazendaz Is there anything that we could help with to get this released? |
|
not really. any one of the core members can release but think we are all pressed for time elsewhere. I can try to look this weekend but do note that I'm spread really thin on things at the moment so if @gtoison or @JuditKnoll could do it, that would work but think they are in same boat at the moment. We are also discussing giving up on java 8 and jumping straight to 17 given we have taken a good deal of time this time around. |
|
Kindly requesting an update - I understand that this is an open source project and we all have other priorities, but it's blocking us @ work and we'd greatly appreciate a release 😅 looks like the milestone is all done. Thanks in advance! |
Could try creating an example to minimally reproduce this, but thought it might be faster to open a PR with the changes I think are needed.
Basically, was trying to get all the
RC_REF_COMPARISONs without needing to list all classes. Found thefindbugs.refcomp.reportAllproperty in the Analysis properties page. Looking at the code, I noticed this property is not read (it was commented out) and added a few lines that would emit the warning if set.Let me know if this makes sense.