Skip to content

Conversation

jurgenvinju
Copy link
Member

@jurgenvinju jurgenvinju commented Nov 8, 2024

This makes the Rascal LSP and the Parametric LSP wrap the values v that come out of the Rascal-based command executors in a ("result" : b) map.

  • that map will be processed by the JSON-RPC bridge and mapped to a {'result' : mapped(b)} object.
  • command executors can now return values of primitive types, like true or 1 or "groetjes", without the JSON-RPC bridge silently failing
  • With value (Command _) as the type signature of all command evaluator functions, Rascal's type system could not check that the return type was not a primitive. Now it does not need to do that anymore.
  • The demo's and I suspect many applications for DLSs would return ("result" : xxx) manually. So all places where that happens we could write return xxx instead.
  • There is code to detect this case of Rascal code still returning ("result":xxx) and then we don't wrap again for backward compatibility.

@DavyLandman
Copy link
Member

DavyLandman commented Nov 8, 2024

I do not feel comfortable with this PR in the current state, since people have used these actions to transfer large rascal values to typescript. Both maps and ADTs will generate a proper json object, and are being used to transfer in a way that the json object maps into a class in typescript.

Maybe we should change the heuristics such that if the value cannot be translated to a proper json root value (aka not a boolean, list or map) than we wrap it with an result map?

Also, this close to a release I propose we'll not be merging this untill after the release, so we get time to test this at the different places we know it's used, or contact our users.

@urbanfly
Copy link
Contributor

I agree with @DavyLandman to not make this breaking change to the command response.

The description mentions

There is code to detect this case of Rascal code still returning ("result":xxx) and then we don't wrap again for backward compatibility.

Let's extend that, like Davy said - if the value is already supported, continue returning it as-is. If the value would have silently failed before, then wrap it in the new structure.

Copy link

@jurgenvinju
Copy link
Member Author

This PR was intended to be backward compatible. I'll write some tests to that end so we can see where this fails to be the case. In my mind it was 🙈

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