search

usethesource / rascal

The implementation of the Rascal meta-programming language (including interpreter, type checker, parser generator, compiler and JVM based run-time system)
http://www.rascal-mpl.org
Other
408 stars 77 forks source link

Fix off-by-one issue in `isOverlapping` #2066

Closed sungshik closed 2 weeks ago

sungshik commented 4 weeks ago

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[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jurgenvinju commented 4 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@sungshik can we add a test for this case?

sungshik commented 3 weeks ago

Tests added!

DavyLandman commented 2 weeks ago

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 commented 2 weeks ago

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