Skip to content

Layout extension trait #627

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 6 commits into from
Jul 16, 2025
Merged

Conversation

frankmcsherry
Copy link
Member

@frankmcsherry frankmcsherry commented Jul 15, 2025

The intent here is to provide a LayoutExt extension trait for types that have a Layout member, to provide them with type aliases and methods on them, e.g. "owned key", "borrowed key", "borrowed to owned function".

This seems to work, but .. there are some mysterious edits needed to unlock the type inference. On that stood out was changing a Tr: Trace + TraceReader< ... > bound into a Tr: Trace< ...> bound. No idea why Rust needed the constraints in that trait, where trait Trace : TraceReader. Perhaps they need to be in the trait at the top of the stack rather than halfway through; I couldn't say.

Hesitant to merge without a better understanding of what is going on, and how much maintenance peril we are in. It's all a bit silly because all we are trying to say is "everyone has the same L: Layout", and .. perhaps the right thing to do is just to add it as a trait argument, e.g. Trace<L>, Batch<L>, Cursor<L>, and life could be better. Though at the same time it really is an associated type: each trait implementations has a single layout in mind.

@frankmcsherry frankmcsherry changed the title WIP: Layout extension trait Layout extension trait Jul 15, 2025
@frankmcsherry frankmcsherry marked this pull request as ready for review July 15, 2025 21:49
@frankmcsherry
Copy link
Member Author

I'm tempted to say that the complexity (and anxiety) about the layout extension trait might justify just eating the complexity of copy/pasting its contents into each of Cursor, BatchReader, and TraceReader, and having the enormous impl blocks. But at least some confidence about how the trait and type resolution works. Pondering now.

@frankmcsherry
Copy link
Member Author

I'm ok merging this, and promise that I'll help undo it if it turns out to be the wrong architecture. But I'm pretty confident that I understand it better now, and that the worst-case scenario is that we inline the LayoutExt types and methods in a few traits, and have a bunch of boilerplate implementations.

@frankmcsherry frankmcsherry merged commit 31e1c69 into TimelyDataflow:master Jul 16, 2025
5 checks passed
@frankmcsherry frankmcsherry deleted the layout_ext branch July 16, 2025 17:34
@github-actions github-actions bot mentioned this pull request Jul 16, 2025
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.

1 participant