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

Add Union Operator; Update AST #69

Merged
merged 13 commits into from
Nov 6, 2024
Merged

Add Union Operator; Update AST #69

merged 13 commits into from
Nov 6, 2024

Conversation

KabirSamsi
Copy link
Contributor

@KabirSamsi KabirSamsi commented Oct 29, 2024

Background

#55 discusses a need for a modified AST that distinguishes between set-to-stream and stream-to-stream policies, and furthermore distinguishes between sets and streams. Within that, we discuss the need for a Union operator, which effectively 'merges several classes into one'. This is critical in the construction of Set-to-Stream transformers, to allow such polices to arbitrate independent of queue distinctions.

Overview

My work on the type system in #63 allowed us to realize that we could turn typechecking into essentially an abstraction, and have our set and stream types inferred at parsing-time itself – that is to say, with a tweak to the AST and extending it to the parser as well. That's the first part part of the PR, which entails adding in the Union constructor to the AST, and partitioning it into the set and stream types.

Caveat And Explanation

@polybeandip and I discussed how far we thought this change should extend. On one hand, modifying Ast.t contractually requires us to modify Policy.t. An alternative, though, is to simply establish a correspondence Ast.t -> Policy.t. For simplicity and to preserve the work done in control, we have kept Policy.t in its original form, and adapted that correspondence.

In turn, this required modifying almost all of our programs such that they would be syntactically correct w.rt. the new AST structure and parsing mechanisms. In addition, a few extra 'reversions' to allow for parsed programs to be correctly evaluated in control.

@KabirSamsi KabirSamsi marked this pull request as draft October 29, 2024 06:08
@KabirSamsi KabirSamsi marked this pull request as ready for review October 29, 2024 16:58
@KabirSamsi KabirSamsi requested review from polybeandip, csziklai and anshumanmohan and removed request for csziklai October 29, 2024 16:58
Copy link
Contributor

@anshumanmohan anshumanmohan left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this; I see it ended up having a largish footprint! I have comment here about how fifo and union relate to other stream-to-stream policies (the example I have commented on involves strict).

progs/incorrect/set_multiple.sched Show resolved Hide resolved
progs/incorrect/unbound_var.sched Show resolved Hide resolved
Copy link
Contributor

@polybeandip polybeandip left a comment

Choose a reason for hiding this comment

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

LGTM! I left a few minor comments down below, but nothing too pressing.

The more I look at code now though, the more I feel like the stuff we do in policy.ml is super hacky lol. I know you made of_program convert from the new AST to the old one at my suggestion, but perhaps your original idea of changing Policy.t was the way to go. Maybe in a future PR, we can get rid of the simulator and that way making that change won't be as painful

rio/frontend/ast.ml Outdated Show resolved Hide resolved
rio/frontend/parser.mly Outdated Show resolved Hide resolved
rio/frontend/policy.ml Show resolved Hide resolved
@KabirSamsi
Copy link
Contributor Author

@polybeandip Can you approve?

@polybeandip polybeandip self-requested a review November 6, 2024 22:57
@KabirSamsi KabirSamsi requested review from anshumanmohan and removed request for anshumanmohan November 6, 2024 23:02
@KabirSamsi KabirSamsi merged commit b9e6d58 into main Nov 6, 2024
3 checks passed
@KabirSamsi KabirSamsi deleted the union-operator branch November 6, 2024 23:02
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