Bugzilla – Full Text Bug Listing
| Summary: | Inconsistent source range for error from build and reconciler (declared package does not match expected) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Markus Keller <markus.kell.r> | ||||
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, satyam.kandula | ||||
| Version: | 3.6.2 | ||||||
| Target Milestone: | 3.8 M6 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 7 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Targetting M6. Created attachment 212050 [details]
proposed fix
Markus, I think the bug is in the editor here, as it is unable to fold the error reported twice but with same range [0,0]. In both reconciler and editor the error range created is [0,0]. You can verify that by changing the call to handle(..) in org.eclipse.jdt.internal.compiler.problem.ProblemReporter.packageIsNotExpectedPackage(CompilationUnitDeclaration) to always take 0,0 as start and end respectively. Even then I see two errors in the editor.
Can you see if this can be fixed in the editor? The above patch will enforce the range to [0,1], but I think the root cause may need to be fixed in the editor too.
No, the editor is not the culprit here. The problem is that there's no clean way to have an IProblem with length 0. IProblem#getSourceStart() and #getSourceEnd() both say that they answer "the start/end position of the problem (inclusive)", so if you create a problem with start/end 0/0, then it actually has length 1. CompilationUnitAnnotationModel#createPositionFromProblem(IProblem) creates the right Position from the reconciler problem. On the other hand, IMarker#CHAR_END is exclusive, so the builder can create markers with length == 0. The inconsistency in JDT Core arises in AbstractImageBuilder#storeProblemsFor(..) line: 733 ... where end == 0 is handled different than all other values. You cannot simply remove the extra case, since that would reintroduce bug 204339. I think the fix should be to remove the extra case in AbstractImageBuilder (revert bug 204339) and decide in ProblemReporter whether start/end can be 0/0 (preferred) or whether they have to be 0/-1 (if compUnitDecl.sourceEnd == 0). This needs to be done for all cases where start/end are just set to 0. Released in master via commit 57b394813576bd0089aca541b5b581458c1b6d75 The ProblemReporter has more cases where end is set to 0. Many look like they can only occur in binary types, but I think some of them should also get protection: abortDueToInternalError(..), cannotReadSource(..), corruptedSignature(..). (In reply to comment #5) > The ProblemReporter has more cases where end is set to 0. Many look like they > can only occur in binary types, but I think some of them should also get > protection: abortDueToInternalError(..), cannotReadSource(..), > corruptedSignature(..). I'd looked at all possible locations inside ProblemReporter and did not see motivation to touch others apart from the 2 I already changed. As you already mentioned, most of them are for binaries. Include corruptedSignature(..) in that. abortDueToInternalError(..) does not produce any warning message at all, but throws an exception usually with an error marker on the CU. Nothing will change here. cannotReadSource(..) raises a warning when a CU's underlying buffer is lost or its encoding is bad. I dont understand what it means to create a problem with a -1 end. No problem marker is actually created for this problem, rather the compilation is aborted with an exception. Anyway, this has always worked with 0,0. We should let the sleeping dog lie. :) Because of these reasons, I thought the current fix is sufficient. Let me know if I missed anything here. OK, thanks for the explanations. I guess we're OK then. The default settings work good but I have run into one failure case. Steps to reproduce. - Have the test case in comment 0. - Ensure that you get only one warning - Now turn off 'Build Automatically' - Move the first line - You will now see two errors coming The error goes off if the file is built. This is a minor bug, can be addressed via a different bug. I just want to check with you before filing a separate bug. (In reply to comment #8) > The default settings work good but I have run into one failure case. > Steps to reproduce. > - Have the test case in comment 0. > - Ensure that you get only one warning > - Now turn off 'Build Automatically' > - Move the first line I cannot reproduce. Can you elaborate on what you mean by 'move the first line'. The error gets stuck on the import statement and wherever i move it, the error moves along. I dont see any second error. (In reply to comment #9) > (In reply to comment #8) > > The default settings work good but I have run into one failure case. > > Steps to reproduce. > > - Have the test case in comment 0. > > - Ensure that you get only one warning > > - Now turn off 'Build Automatically' > > - Move the first line > I cannot reproduce. Can you elaborate on what you mean by 'move the first > line'. The error gets stuck on the import statement and wherever i move it, the > error moves along. I dont see any second error. The reconciler still shows me at the first line. (In reply to comment #10) > The reconciler still shows me at the first line. Confirmed with Satyam that this is an existent issue and has nothing to do with the fix. Please file a separate bug if desired. I can reproduce comment 8 when I move the first line with Alt+Down, Alt+Up. But that's just a limitation of what we can do when you choose to have the reconciler enabled but auto-build disabled. When you move source ranges, we try to keep markers close to their original location. This cannot be precise, though. If the distance is big enough, we also can't collapse the marker and the reconciler problems into a single problem, and that's why you can get the reconciler problem plus a stale marker problem (gray-on-white icon). I doubt there's much we could improve here. (In reply to comment #12) > I can reproduce comment 8 when I move the first line with Alt+Down, Alt+Up. > > But that's just a limitation of what we can do when you choose to have the > reconciler enabled but auto-build disabled. > > When you move source ranges, we try to keep markers close to their original > location. This cannot be precise, though. If the distance is big enough, we > also can't collapse the marker and the reconciler problems into a single > problem, and that's why you can get the reconciler problem plus a stale marker > problem (gray-on-white icon). > > I doubt there's much we could improve here. I understand. So, I am not even opening a new bug too. Verified for 3.8M6 using build I20120312-1300 |
HEAD, but already broken in 3.6.2. Create this CU in a non-default package: import java.util.Date; public class C { Date d; } => The editor shows 2 errors in this CU, one from the reconciler with range [0, 1] and one from the builder with range [0, 0]. Message: The declared package "" does not match the expected package "p". The two errors should have the same range so that the editor can fold them together. The range [0, 1] is a bit better, since it shows up in the source and allows to show the quick fix hover.