Skip to content

1340: Changing RenderContext from an inner class to a nested class with its own generics #1341

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

Merged
merged 2 commits into from
Jun 10, 2025

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Jun 10, 2025

Yes, this means we have to provide type parameters every time we use it.

However, it also means we won't break the out RenderingT variance (which will not be allowed come the K2 compiler).

This will fix #1340 as we will no longer be consuming RenderingT (via the Workflow instance to get the PropsT and OutpuT types) when we pass RenderContext into render() as its types - for PropsT and OutpuT will be provided explicitly.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/render-context-leak branch from 4f14d12 to cdf8890 Compare June 10, 2025 19:46
@steve-the-edwards steve-the-edwards force-pushed the sedwards/move-render-context branch from 4a09419 to 8ba5356 Compare June 10, 2025 19:47
@steve-the-edwards steve-the-edwards changed the title Changing RenderContext from an inner class to a nested class with its own generics 1340: Changing RenderContext from an inner class to a nested class with its own generics Jun 10, 2025
@@ -37,7 +37,7 @@ class HelloTerminalWorkflow : TerminalWorkflow,
override fun render(
renderProps: TerminalProps,
renderState: State,
context: RenderContext
context: StatefulWorkflow.RenderContext<TerminalProps, State, ExitCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the StatefulWorkflow prefix in some spots but not others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think just sloppiness in my regex replacing 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the StatefulWorkfow.RenderContext usage FWIW. but in most cases I think its unambiguous, so no need to be too strict.

@rjrjr
Copy link
Contributor

rjrjr commented Jun 10, 2025

Will breaking it out allow us to follow up and undo any of the recent c*variance work?

@steve-the-edwards steve-the-edwards force-pushed the sedwards/move-render-context branch from 8ba5356 to bd4480b Compare June 10, 2025 20:07
Base automatically changed from sedwards/render-context-leak to main June 10, 2025 20:09
@steve-the-edwards
Copy link
Contributor Author

Will breaking it out allow us to follow up and undo any of the recent c*variance work?

It's possible we might be able to get in PropsT back. But definitely not out OutputT. as we pass RenderContext<PropsT, StateT, OutputT> as a parameter of render(), which means, by definition the Workflow instance is a consumer of OutputT and it should not be allowed to be covariant across OutputT.

I could do a test bringing back in PropsT.

The other thing this does do though is make RenderingT safe. We would have actually had to make that invariant without this change. So that is already a win!

@steve-the-edwards steve-the-edwards marked this pull request as ready for review June 10, 2025 20:12
@steve-the-edwards
Copy link
Contributor Author

Note also that I can cache RenderContext much more effectively and cleanly now as well - #1342

@steve-the-edwards
Copy link
Contributor Author

Will breaking it out allow us to follow up and undo any of the recent c*variance work?

It's possible we might be able to get in PropsT back. But definitely not out OutputT. as we pass RenderContext<PropsT, StateT, OutputT> as a parameter of render(), which means, by definition the Workflow instance is a consumer of OutputT and it should not be allowed to be covariant across OutputT.

I could do a test bringing back in PropsT.

The other thing this does do though is make RenderingT safe. We would have actually had to make that invariant without this change. So that is already a win!

Nope. I checked and we can't do in PropsT because of this API in BaseRenderContext:

actionSink: Sink<WorkflowAction<PropsT, StateT, OutputT>>

PropsT is being produced there, so it can't be in PropsT on RenderContext, which means it can't anywhere else.

@steve-the-edwards steve-the-edwards merged commit 9973936 into main Jun 10, 2025
45 checks passed
@steve-the-edwards steve-the-edwards deleted the sedwards/move-render-context branch June 10, 2025 20:40
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.

RenderingT still has unsafe variance
2 participants