Community
Participate
Working Groups
JDT Core overwrites Status.matches(int) in a breaking way. The JavaModelStatus.matches(int) ignores the severity that has been set on the status.
It also violates org.eclipse.core.runtime.IStatus.isOK().
Dani, please spell out/clarify what exactly you are talking about so we are on the same page - TIA.
(In reply to comment #2) > Dani, please spell out/clarify what exactly you are talking about so > we are on the same page - TIA. Srikanth, are you asking to clarify what/why it is broken, or rather how it should be fixed?
(In reply to comment #3) > (In reply to comment #2) > > Dani, please spell out/clarify what exactly you are talking about so > > we are on the same page - TIA. > > Srikanth, are you asking to clarify what/why it is broken, or rather how it > should be fixed? The former part Dani.
(In reply to comment #2) > Dani, please spell out/clarify what exactly you are talking about so > we are on the same page - TIA. The API specification of IStatus.matches(int) is: * Returns whether the severity of this status matches the given * severity mask. Note that a status with severity <code>OK</code> * will never match; use <code>isOK</code> instead to detect * a status with a severity of <code>OK</code>. * * @param severityMask a mask formed by bitwise or'ing severity mask * constants (<code>ERROR</code>, <code>WARNING</code>, * <code>INFO</code>, <code>CANCEL</code>) * @return <code>true</code> if there is at least one match, * <code>false</code> if there are no matches but JavaModelStatus.matches(int) completely ignores the severity. It does some bit juggling on the code instead. This is what the API of IStatus.isOK() defines: * Returns whether this status indicates everything is okay * (neither info, warning, nor error). * * @return <code>true</code> if this status has severity * <code>OK</code>, and <code>false</code> otherwise and JavaModelStatus.isOK() does not respect that: it looks at the code instead of at the severity.
(In reply to comment #5) > (In reply to comment #2) > This is what the API of IStatus.isOK() defines: > > * Returns whether this status indicates everything is okay > * (neither info, warning, nor error). > * > * @return <code>true</code> if this status has severity > * <code>OK</code>, and <code>false</code> otherwise > > and JavaModelStatus.isOK() does not respect that: it looks at the code instead > of at the severity. Looking at how the singleton object org.eclipse.jdt.internal.core.JavaModelStatus.VERIFIED_OK is defined, it appears that org.eclipse.core.runtime.IStatus.OK is overloaded and reused (integers codes making it easier to do this as opposed to type safe enumerations) to indicate a status of all being alright. I think it is worth investigating, but I suspect being the only the producer of java model status objects, we are internally consistent despite this type looseness. matches: I didn't check this one, but perhaps the case is similar. We seem to assume a code of OK implies a severity of OK and I don't know if there are counter examples that exist currently.
(In reply to comment #5) [...] > but JavaModelStatus.matches(int) completely ignores the severity. It does some > bit juggling on the code instead. [...] > and JavaModelStatus.isOK() does not respect that: it looks at the code instead > of at the severity. Jay/Dani, Can we construct a case where we can actually see any broken behavior purely from an API standpoint: i.e without having looked at the implementation, could we have concluded something is amiss here purely by observing behavior ?
> Jay/Dani, Can we construct a case where we can actually see any broken > behavior purely from an API standpoint: i.e without having looked at the > implementation, could we have concluded something is amiss here purely by > observing behavior ? We ran into this with the fix for bug 287164 where JDT Core currently patches the API by creating a subclass of JavaModelStatus. Also, every JDT Core developer who constructs a JavaModelStatus with JavaModelStatus.JavaModelStatus(int, int, IJavaElement, IPath, String) will run into this as soon as he uses a severity that doesn't match with the code, e.g. if he uses a (error) code to signal a warning. In addition I could not find any documentation that would indicate that the code is somehow linked to the severity or that could explain the implementation in #getBits() and #matches(JavaModelStatus, int).
(In reply to comment #8) > In addition I could not find any documentation that would indicate that the > code is somehow linked to the severity or that could explain the implementation > in #getBits() and #matches(JavaModelStatus, int). True, I had trouble understanding the code and the relevance too. I will try to come up with a failing test.
Created attachment 206771 [details] Test for IJavaModelStatus.matches() The attached test case checks a particular status against different severity. The following is the status that is being tested and results of matches() calls against IStatus.OK, ERROR, WARNING and INFO. Java Model Status [Unbound classpath variable: 'JCL_LIB' in project 'P1'] isError:false isWarning:true isInfo :false isOK:true The status is an ERROR and only isError should return true here. I don't yet know what's happening.
The following change to the matches() makes the previous test produce the expected results. And all the existing model tests pass. protected boolean matches(JavaModelStatus status, int mask) { return (status.getSeverity() == mask); //int severityMask = mask & 0x7; //int categoryMask = mask & ~0x7; //int bits = status.getBits(); //return ((severityMask == 0) || (bits & severityMask) != 0) && ((categoryMask == 0) || (bits & categoryMask) != 0); } I wonder what all the bitwise operations in matches() and getBits() are for.
(In reply to comment #11) > The following change to the matches() makes the previous test produce the > expected results. And all the existing model tests pass. Then you need more test cases ;-). The following is a good one: s= new JavaModelStatus(IStatus.OK, some code, "shows bug"); assert: s.isOK().
(In reply to comment #12) > (In reply to comment #11) > > The following change to the matches() makes the previous test produce the > > expected results. And all the existing model tests pass. > > Then you need more test cases ;-). > > The following is a good one: > > s= new JavaModelStatus(IStatus.OK, some code, "shows bug"); > assert: s.isOK(). Right. But being the sole producer of these status objects, I don't think we ever create such objects i.e we are internally consistent.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > The following change to the matches() makes the previous test produce the > > > expected results. And all the existing model tests pass. > > > > Then you need more test cases ;-). > > > > The following is a good one: > > > > s= new JavaModelStatus(IStatus.OK, some code, "shows bug"); > > assert: s.isOK(). > > Right. But being the sole producer of these status objects, I don't think > we ever create such objects i.e we are internally consistent. That's good. But given the lack of documentation a developer can easily produce such code and then we're no longer good. The least thing we need to do for this bug is to improve the doc and also explain the juggling with the code value.
Since Jay had to go on unavoidable unplanned time off for several days, this is not ready: retargetting to 3.8 M5.
This is not a recent regression and is a long standing issue. As Jay is busy with Orion for now, moving this out of M5. Will include in plan a later time as cycles become available.
(In reply to comment #16) > This is not a recent regression and is a long standing issue. > As Jay is busy with Orion for now, moving this out of M5. > Will include in plan a later time as cycles become available. That's fine with me.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
