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

Refactor Value into command and expression values #1047

Closed
byorgey opened this issue Jan 25, 2023 · 1 comment · Fixed by #1928
Closed

Refactor Value into command and expression values #1047

byorgey opened this issue Jan 25, 2023 · 1 comment · Fixed by #1928
Labels
C-Project A larger project, more suitable for experienced contributors. G-CESK machine This issue has to do with the CESK machine (the Swarm language interpreter). S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact.

Comments

@byorgey
Copy link
Member

byorgey commented Jan 25, 2023

Overall I'm not happy with the way we handle VResult. It is a fertile source of bugs (#977 , #858, #327, #328) and will probably continue to be so because of some subtle invariants. It's necessary internally in order to pass along intermediate results that also carry an environment of bound names, but there are also many places where we don't expect to see it and don't want to have to worry about dealing with it. A few options for improving the situation that come to mind:

  1. Remove VResult from Value, and create a wrapper type for "values with environment" (EValue?) which can be either a Value or a VResult, and carefully decide which instances of the Value type should change to EValue.
  2. Add a type parameter to Value that indicates whether it is allowed to contain VResult or not.

Originally posted by @byorgey in #977 (comment)

Note that the distinction is precisely between values which result from evaluating an expression (which can never be a VResult) and values which result from executing a command (which can be).

@byorgey
Copy link
Member Author

byorgey commented Nov 12, 2023

Note that the idea with #1087 is that we won't need VResult at all any more.

@byorgey byorgey added C-Project A larger project, more suitable for experienced contributors. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. G-CESK machine This issue has to do with the CESK machine (the Swarm language interpreter). labels Nov 12, 2023
@mergify mergify bot closed this as completed in #1928 Jun 19, 2024
@mergify mergify bot closed this as completed in 23b5398 Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Project A larger project, more suitable for experienced contributors. G-CESK machine This issue has to do with the CESK machine (the Swarm language interpreter). S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant