refactor: change access types to BindingAccess #254

grjte · 2023-04-14T09:45:56Z

Currently, there are 5 different variants of Expression into which references to bound identifiers within variables or constraints are parsed - Elem, VectorAccess, MatrixAccess, Rand, and TraceBindingAccess. These are all handled in essentially the same way within the IR, so consolidating these into a single type allows us to simplify handling in the IR, especially of variables.

This PR does the following. Because of the nature of the change, it's hard to split it into a smaller change without errors.

introduce BindingAccess
replace Elem, VectorAccess, and MatrixAccess with BindingAccess
update codegen handling for new Value types

Future items for discussion:

Expression::Rand should probably be similarly replaced. This would be fairly straightforward.
Ideally, we would consolidate TraceBindingAccess into BindingAccess as well. This would improve the code clarity significantly, since all trace bindings for the "current" row are already parsed as BindingAccess, so TraceBindingAccess only represents references to trace bindings in the "next" row or being accessed as a Range. This means that the handling within the IR of accessing trace bindings is split into 2 places. The complication with this is that we access ranges of the trace, so AccessType would need to change, and I'm not sure of the best way to adjust this.

bobbinth

Thank you! I think this improves things quite a lot! I left some thoughts/comments inline. It is possible that I didn't consider all the implications for some of the - so, let me know if I missed anything.

For some of them, it may make sense to address in future PRs not to complicate this PR even further.

air-script-core/src/access.rs

bobbinth · 2023-04-15T06:46:00Z

air-script-core/src/expression.rs

    Const(u64),
-    /// Represents any named constant or variable.
-    Elem(Identifier),
-    /// Represents an element inside a constant or variable vector. [VectorAccess] contains the
-    /// name of the vector and the index of the element to access.
-    VectorAccess(VectorAccess),
-    /// Represents an element inside a constant or variable matrix. [MatrixAccess] contains the
-    /// name of the matrix and indices of the element to access.
-    MatrixAccess(MatrixAccess),
+    /// Represents a reference to all or part of a constant, variable, or trace binding.
+    BindingAccess(BindingAccess),
    TraceAccess(TraceAccess),
    TraceBindingAccess(TraceBindingAccess),
    /// Represents a random value provided by the verifier. The first inner value is the name of


I think we can probably get rid of TraceAccess, TraceBindingAccess, and Rand from here. I'm not sure they need any special handling as compared to regular variables. We should be able to say that something like $main[2] is BindingAccess { name = "$main", access = Vector(2) }.

Same could apply to everything else except for something like $main[2]'. To address this, I think we can modify BindingAccess to look something like this:

pub struct BindingAccess { name: Identifier, access: AccessType, offset: usize, }

So, anything of the form <expr>' would have offset = 1 and just <expr> would have offset = 0. Whether non-zero offset is valid against a given symbol could be validated at symbol resolution time.

A couple of other minor things:

Should we rename Const to Constant? I think we have it as Constant in some other places.

I don't love the name BindingAccess. Maybe SymbolAccess would be better. The Expression enum could look like this:

pub enum Expression { Constant(u64), SymbolAccess(SymbolAccess), Add(Box<Expression>, Box<Expression>), Sub(Box<Expression>, Box<Expression>), Mul(Box<Expression>, Box<Expression>), Exp(Box<Expression>, Box<Expression>), ListFolding(ListFolding), }

Not a strong preference though and if you'd prefer to keep BindingAccess I'm fine with that too.

I'm fine with renaming BindingAccess to SymbolAccess, but I think it would be cleaner to do it in a separate PR.

I agree that I'd like to remove Rand and TraceBindingAccess and just use BindingAccess (or SymbolAccess), but I think this will require a lot of changes and should be addressed in a separate PR as well. Adding offset to the BindingAccess struct should resolve the complication I mentioned in the PR description.

We shouldn't remove TraceAccess, as it serves a different function. It is the type used in Value nodes which are leaves in the AlgebraicGraph. However, we can remove it from use in the parser so it is only used in the IR. I would also do this in a separate PR.

air-script-core/src/access.rs

air-script-core/src/trace.rs

ir/src/constraint_builder/expression.rs

bobbinth · 2023-04-15T07:29:23Z

ir/src/constraint_builder/variables.rs

+        AccessType::Vector(new_idx) => match expr {
+            Expression::BindingAccess(inner_binding) => match inner_binding.access_type() {
+                AccessType::Default => {
+                    let new_binding_access = BindingAccess::new(
+                        inner_binding.ident().clone(),
+                        AccessType::Vector(new_idx),
+                    );
+                    Ok(Expression::BindingAccess(new_binding_access))
+                }


I got lost in this a little. Does this mean that if we have something like:

let x = [1, 2, 3] let c = x[2]

We replace c = x[2] with c = 3?

No, it means we replace c where it is used by x[2]. In other words, if we had something like the following:

let x = [1, 2, 3] let c = x[2] enf c = 3

this step turns it into:

let x = [1, 2, 3] let c = x[2] enf x[2] = 3

Then a subsequent processing turns it into enf 3 = 3

bobbinth · 2023-04-15T07:30:19Z

ir/src/constraint_builder/variables.rs

+        AccessType::Matrix(row_idx, col_idx) => match expr {
+            Expression::BindingAccess(inner_binding) => match inner_binding.access_type() {
+                AccessType::Default => {
+                    let new_binding_access = BindingAccess::new(
+                        inner_binding.ident().clone(),
+                        AccessType::Matrix(row_idx, col_idx),
+                    );
+                    Ok(Expression::BindingAccess(new_binding_access))


Similarly, I'm not sure I understood what's happening here.

ir/src/symbol_table/symbol_type.rs

tohrnii

Thank you @grjte, this PR makes things a lot more coherent. I don't have much to add other than what @bobbinth pointed out. I just left a couple of very minor optional nits.

air-script-core/src/trace.rs

ir/src/constraint_builder/variables.rs

- add BindingAccess - replace Elem, VectorAccess, and MatrixAccess with BindingAccess - update codegen handling for new Value types

grjte · 2023-04-17T10:41:59Z

Thank you both for the review! I have addressed the smaller points. I agree with the larger points, and appreciate the suggestions for addressing the consolidation of Rand and TraceBindingAccess into BindingAccess. As these adjustments will result in significant changes, I'm going to leave them for future refactoring PRs. I'm planning the following series of PRs:

simple renaming (e.g. BindingAccess to SymbolAccess and SymbolType to SymbolBinding)
replace Rand
replace TraceBindingAccess
revisit the variable handling for improving clarity again (will probably be required as part of replacing TraceBindingAccess)

grjte requested review from bobbinth, tohrnii and Fumuran April 14, 2023 09:53

bobbinth reviewed Apr 15, 2023

View reviewed changes

tohrnii approved these changes Apr 17, 2023

View reviewed changes

air-script-core/src/trace.rs Outdated Show resolved Hide resolved

ir/src/constraint_builder/variables.rs Outdated Show resolved Hide resolved

refactor: change access types to BindingAccess

86bc466

- add BindingAccess - replace Elem, VectorAccess, and MatrixAccess with BindingAccess - update codegen handling for new Value types

grjte force-pushed the grjte-refactor-clarity-3 branch from d9f1934 to 86bc466 Compare April 17, 2023 10:38

grjte merged commit 48373cf into next Apr 17, 2023

grjte deleted the grjte-refactor-clarity-3 branch April 17, 2023 10:42

grjte mentioned this pull request Apr 21, 2023

IR refactorings post v0.2 #150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change access types to BindingAccess #254

refactor: change access types to BindingAccess #254

grjte commented Apr 14, 2023 •

edited

Loading

bobbinth left a comment

bobbinth Apr 15, 2023

grjte Apr 17, 2023

bobbinth Apr 15, 2023

grjte Apr 17, 2023

bobbinth Apr 15, 2023

tohrnii left a comment •

edited

Loading

grjte commented Apr 17, 2023

refactor: change access types to BindingAccess #254

refactor: change access types to BindingAccess #254

Conversation

grjte commented Apr 14, 2023 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Apr 15, 2023

Choose a reason for hiding this comment

grjte Apr 17, 2023

Choose a reason for hiding this comment

bobbinth Apr 15, 2023

Choose a reason for hiding this comment

grjte Apr 17, 2023

Choose a reason for hiding this comment

bobbinth Apr 15, 2023

Choose a reason for hiding this comment

tohrnii left a comment • edited Loading

Choose a reason for hiding this comment

grjte commented Apr 17, 2023

grjte commented Apr 14, 2023 •

edited

Loading

tohrnii left a comment •

edited

Loading