Bugzilla – Full Text Bug Listing
| Summary: | NPE in workflow during validation if location information is missing | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] TMF | Reporter: | Daniel Weber <daniel.weber.dev> | ||||||||
| Component: | Xtext | Assignee: | Project Inbox <tmf.xtext-inbox> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | clay, f.tomassetti, sebastian.zarnekow | ||||||||
| Version: | unspecified | Flags: | sebastian.zarnekow:
indigo+
|
||||||||
| Target Milestone: | M5 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 176644 [details]
proposed patch
I refuse the patch as nullsafeCompare seems to complicated to me and Integer.MAX and Integer.MIN suggest something that's not in sync with the contract of comparable. I'ld rather like a simple implementation like:
int compare(Integer i, Integer j) {
i = ensureNotNull(i);
j = ensureNotNull(j);
return i.compareTo(j);
}
Integer ensureNotNull(Integer i) {
return i != null ? i : Integer.valueOf(-1);
}
Michael, will you provide an updated patch? yes, later today (In reply to comment #3) > Michael, will you provide an updated patch? Created attachment 177247 [details]
revised patch (not tested)
Scheduled to ensure a review of the attached patch. There is workaround to use until the parch is not inserted in the released version? Michael, are you still working on this one? The patch is still marked as "not tested"? Michael, any comment on comment #8? (In reply to comment #9) > Michael, any comment on comment #8? Sebastian, i will come up with some tests for this patch in the next days if it's ok for you. That would be great. Created attachment 185362 [details]
patch + test
Hi Michael, thanks for the update. The implementation looks good to me. Some minor remarks: - getDiagnosticComparator should return a Comparator<MWEDiagnostic> - MWEDiagnosticComparator should be public Making MWEDiagnosticComparator public has the advantage that it can be tested independently thus poluting the IndexTestLanguage-module would not be necessary. Simply comparing inconsistent diagnostics would be sufficient to proof the fix to be valid. However, from my point of view the patch is ok besides the aforementioned two minor issues. pushed to master Closing all bugs that were set to RESOLVED before Neon.0 Closing all bugs that were set to RESOLVED before Neon.0 |
Build Identifier: I20100608-0911 Validation runs into a NPE when there is no location information for the "erroneous object". Reproducible: Always Steps to Reproduce: 1. Create a new Xtext project using the default values, including a generator project 2. Add the following to the generated Validator @Check public void checkGreetingStartsWithCapital(Greeting greeting) { Greeting objectWithoutLocation = MyDslFactory.eINSTANCE.createGreeting(); error("Name should start with a capital", objectWithoutLocation, MyDslPackage.GREETING__NAME); } 3. Run the grammar generator 4. Run the generator workflow from the generator project ==> Caused by: java.lang.NullPointerException at org.eclipse.xtext.mwe.Validator$1$1.compare(Validator.java:148) at org.eclipse.xtext.mwe.Validator$1$1.compare(Validator.java:1) at java.util.TreeMap.compare(TreeMap.java:1093) at java.util.TreeMap.put(TreeMap.java:465) at java.util.TreeSet.add(TreeSet.java:210) This is because linenumber is null for this issue: if (issue1.getLineNumber() < issue2.getLineNumber()) And probably the same for the offset (a few lines below in the Validator class). The org.eclipse.xtext.validation.DiagnosticConverterImpl seems to handle missing location information just fine: Triple<Integer, Integer, Integer> locationData = getLocationData(diagnostic); if (locationData != null) { issue.setLineNumber(locationData.getFirst()); issue.setOffset(locationData.getSecond()); issue.setLength(locationData.getThird() - issue.getOffset()); } So org.eclipse.xtext.mwe.Validator should probably handle this as well. We actually ran into this because we added issues for imported non-Xtext resources (XMI), for which getLocationData does not work.