Fix off-by-one issue in isOverlapping

[sungshik]

Before this PR:

rascal>isOverlapping(|unknown:///|(0,2), |unknown:///|(2,1))
bool: true

After this PR:

rascal>isOverlapping(|unknown:///|(0,2), |unknown:///|(2,1))
bool: false

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49%. Comparing base (0eea46e) to head (904f0be).
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##              main   #2066   +/-   ##
=======================================
  Coverage       49%     49%           
- Complexity    6296    6302    +6     
=======================================
  Files          664     664           
  Lines        59632   59632           
  Branches      8648    8648           
=======================================
+ Hits         29462   29470    +8     
+ Misses       27954   27951    -3     
+ Partials      2216    2211    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jurgenvinju]

Excellent fix.

Right, offset 0 length 2 means columns 0 to 1 inclusive, so 2 is not in that range and would not overlap. Just logging the thinking here.

This fix may have some unforseen impact in code that compensates for the bug at (indirect) call sites.

I recommend we test this with rascal-core running with this implementation on the standard library and the big tests at least once before merging. If there are no problems: merge. If there are many, we could fix those simultaneously and then merge.

[DavyLandman]

@PaulKlint has done this by now using the big-test script, which always takes the latest main commit from rascal (unless configured otherwise). a wait, it's not merged yet. we could point the script at this branch.

[DavyLandman]

@sungshik can we add a test for this case?

[sungshik]

Tests added!

[DavyLandman]

how about moving this to this already existing file with location tests?

rascal/src/org/rascalmpl/library/lang/rascal/tests/basic/Locations.rsc

Lines 437 to 448 in cbba43c

	// isOverlapping
	test bool isOverlapping1(int _){
	<l1, l2> = makeLocsWithGap(-1);
	return report(l1, l2, isOverlapping(l1, l2));
	}
	test bool isOverlapping2(int _){
	<l1, l2> = makeLocsWithGap(10);
	return !isOverlapping(l1, l2);
	}

[sungshik]

Ok, sure, I'll do.

For future reference, though, which tests go in lang::rascal::tests::basic::* and which tests go in lang::rascal::tests::library::*? I was assuming that tests related to language primitives would go in basic::* and tests for library functions in library::*, but I now also notice for instance:

rascal/src/org/rascalmpl/library/lang/rascal/tests/basic/Booleans.rsc

Line 40 in 6fa3b43

test bool tstArbBool() { b = arbBool() ; return b == true || b == false; }

And:

rascal/src/org/rascalmpl/library/lang/rascal/tests/library/Boolean.rsc

Lines 25 to 28 in 6fa3b43

	test bool arb2(){
	bool B = arbBool();
	return (B == true) || (B == false);
	}

[DavyLandman]

I have no good answer for this. Maybe @jurgenvinju or @PaulKlint remembers the reason for the split?

In general, I just look at where other functions from the same module are tested.

[PaulKlint]

I am the first to admit that this organization can (and should) be improved, but the ratio is this:

• tests for languages features go to basic, functionality or one of the other more specialized directories
• tests for the standard libraries go to library

As a consequence, tests for, say, locations can be found in two places. Not very good. The problem is that there are several dimensions that can be used to organize the tests:

• syntactic category (e.g., expression, function, pattern, extend, import)
• syntactic alternative (e.g., call, addition, is-defined)
• language feature (e.g., dynamic dispatch, overloading, memoization, parsing)
• data types (e.g., bool, int, map).
• library modules (e.g., Location)

In many cases there is overlap between the categories. Come up with a better organization and I will be happy to implement it.

[sungshik]

Ok, thanks for clarifying!

[DavyLandman]

Follow this run to see if it breaks new things: https://github.com/usethesource/rascal-core-big-tests/actions/runs/11720235621

although I would expect the rename code in vs code to be more impacted by changes in this logic.

[DavyLandman]

All the test pass, also the big rascal-core. So merging this.