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

Parser origin location was not forwarded properly #2037

Closed PieterOlivier closed 2 weeks ago

PieterOlivier commented 1 month ago

The parse function has an extra parameter in case the origin was not the same as the input location. But the origin was never properly passed on.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 49%. Comparing base (fc72dc7) to head (5cc0b90). Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
...g/rascalmpl/values/RascalFunctionValueFactory.java 0% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2037 +/- ## ======================================= Coverage 49% 49% - Complexity 6306 6318 +12 ======================================= Files 664 664 Lines 59635 59632 -3 Branches 8647 8648 +1 ======================================= + Hits 29471 29496 +25 + Misses 27946 27924 -22 + Partials 2218 2212 -6 ```

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

DavyLandman commented 1 month ago

@jurgenvinju do you remember if this was intentional? it seems to me like a small bug, but there might have been an explicit reason for it to ignore that origin parameter?

jurgenvinju commented 2 weeks ago

The reason for this extra parameter was the overloads on the generated parse function. One has a str as first parameter and the second has LOC as first parameter. The second loc parameter is only important for the str case.

We couldn't catch this with a keyword parameter because those couldn't be expressed in function types. And so we ended up with this mixed solution.

The bug is well identified still. I guess we never triggered it because only the above two use cases are in vogue. Still could be useful to have this. Tricky since it breaks the contract between the actual file and the src origins in the tree!

Thinking about this I'd rather assert that both locations are the same than override one with the other. Throw an illegal argument exception otherwise. Consider the havoc different locs would do to the semantics of the LSP servers, for example.

jurgenvinju commented 2 weeks ago

The str case is meant for experimenting on the repl and testing and debugging purposes.

PieterOlivier commented 2 weeks ago

I have changed to approach so now een IllegalArgumentException is thrown when there is a mismatch between origin and input.

jurgenvinju commented 2 weeks ago

Thanks