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

Runtime: warn when an expression doesn't return state #832

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

doc-han
Copy link
Collaborator

@doc-han doc-han commented Nov 26, 2024

Short Description

This PR

  1. warns when an operation doesn't return a state. Warning happens when an operation realizes that the one before it didn't return state. runtime: crash occurs if an expression doesn't return state #737

Fixes #737 #761

Implementation Details

when an operation fails to return state.

Before

Operation Result
Screenshot 2024-11-26 at 2 25 35 AM Screenshot 2024-11-26 at 2 27 03 AM
the following operations just fail without us knowing why

After

Operation Result
Screenshot 2024-11-26 at 2 25 35 AM Screenshot 2024-11-26 at 2 27 03 AM
we warn that the previous operation didn't return a state & might cause the current to fail

QA Notes

List any considerations/cases/advice for testing/QA here.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

@doc-han doc-han force-pushed the farhan/expression-doesnt-return-state branch from 93e1325 to 889fdd6 Compare November 26, 2024 07:35
@doc-han doc-han requested a review from josephjclark November 26, 2024 08:26
@doc-han
Copy link
Collaborator Author

doc-han commented Nov 26, 2024

yet to add some additional tests!

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic @doc-han, thank you so much! It's a really good, clean PR and you've done all the right things.

Having had a little play with this and the output, I'm having some thoughts about the structure.

At the end of an operation, if no state was returned, let's always logger.debug(warning: operation ${name} did not return state`). That gives us a nice log in-context, and because it's debug level we can easily hide it.

So the output will be like:

[R/T] ❯ Starting operation 1
[R/T] ❯ Warning: operation 1 did not return state
[R/T] ❯ Operation 1 complete in 0ms

Plus there are some other comments

packages/runtime/src/util/null-state.ts Outdated Show resolved Hide resolved
packages/runtime/src/util/null-state.ts Show resolved Hide resolved
packages/runtime/src/util/null-state.ts Outdated Show resolved Hide resolved
packages/runtime/test/execute/step.test.ts Outdated Show resolved Hide resolved
packages/runtime/src/execute/expression.ts Show resolved Hide resolved
packages/runtime/src/execute/expression.ts Outdated Show resolved Hide resolved
packages/runtime/src/execute/expression.ts Outdated Show resolved Hide resolved
@doc-han
Copy link
Collaborator Author

doc-han commented Nov 26, 2024

I think most of our user errors will be reduced extremely when they have development time help(proper static & semantic validations)

@doc-han doc-han changed the title Runtime: warn when an expression doesn't return state or not a fn Runtime: warn when an expression doesn't return state Nov 27, 2024
@doc-han doc-han requested a review from josephjclark November 27, 2024 16:58
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment!

@doc-han doc-han force-pushed the farhan/expression-doesnt-return-state branch 2 times, most recently from f142839 to f7541df Compare December 2, 2024 14:12
@doc-han doc-han force-pushed the farhan/expression-doesnt-return-state branch from f7541df to 8032762 Compare December 2, 2024 15:04
@doc-han doc-han force-pushed the farhan/expression-doesnt-return-state branch from 8032762 to 406926e Compare December 2, 2024 15:17
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic @doc-han , thanks

One little tweak please, then can I ask you to run pnpm changeset and generate a runtime patch log?

Then I can get this merged and released tomorrow 🎉

packages/runtime/src/execute/step.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll do a final check and release in the morning 🚀

@josephjclark josephjclark merged commit 82afe62 into main Dec 3, 2024
7 checks passed
@josephjclark josephjclark deleted the farhan/expression-doesnt-return-state branch December 3, 2024 11:19
@doc-han doc-han mentioned this pull request Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

runtime: crash occurs if an expression doesn't return state
2 participants