Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parser origin location was not forwarded properly #2037

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

PieterOlivier
Copy link
Contributor

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.

Copy link

codecov bot commented Oct 3, 2024

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 ⚠️
Additional details and impacted files
@@           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     

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

@DavyLandman
Copy link
Member

@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
Copy link
Member

jurgenvinju commented Nov 13, 2024

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
Copy link
Member

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

@PieterOlivier
Copy link
Contributor Author

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

@jurgenvinju
Copy link
Member

Thanks

@PieterOlivier PieterOlivier merged commit 42cf801 into main Nov 13, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants