Add support for parsing of constants #63

tohrnii · 2022-11-04T14:24:39Z

Partly addressing #60.

This PR adds support for constants to the parser. This doesn't yet produce valid IR for constants. That will be handled in
#67.

Questions:

Should we enforce using CAPITALS for constant names? If yes, should we throw an error or show a warning to the user?
Should we have any special characters for accessing constant values to ensure we know what the element is while building the AST instead of knowing after cross referencing it in the symbol table while building IR?

bobbinth · 2022-11-15T06:09:55Z

Should we enforce using CAPITALS for constant names? If yes, should we throw an error or show a warning to the user?

I'm in favor of this. It is kind of consistent with Rust. But also, I don't feel strongly about this.

Should we have any special characters for accessing constant values to ensure we know what the element is while building the AST instead of knowing after cross referencing it in the symbol table while building IR?

What would be an example of using special characters here? If it is prefixing every constant reference with a special character, I don't think we need it (especially if we use capital letters for constant names).

tohrnii · 2022-11-15T06:25:12Z

If it is prefixing every constant reference with a special character, I don't think we need it (especially if we use capital letters for constant names).

Agreed. I meant that as an option if we don't go with using CAPITALS. But agreed that this doesn't seem like a good solution. I have gone with using a generic enum type in this PR while building the AST (for eg VecElem to access any kind of vector element (constant, variable, public input, etc)) instead of using specific enum type like PubInput for public input value.

grjte · 2022-11-25T12:30:34Z

I think there's a general question here about whether we should be using column-major or row-major form. I know Miden is column-major, but Winterfell isn't always, and many other projects may not be. Row-major is more common in general, and we are aiming for AirScript to have more general utility than just for Miden.

@bobbinth curious about your thoughts

grjte

Thanks for your work @tohrnii! I've left several comments inline, mostly about ways to use more shared abstractions and reduce near-duplications.

One other comment, though - the tests are mostly for the perfect case, which makes them great as examples and sanity checks, but doesn't give us the level of confidence in the code that I think we'd like to have. Can you add more edge cases for all of them? I added examples in one case, but the other test modules should be updated too

parser/src/ast/constants.rs

grjte · 2022-11-25T11:51:03Z

parser/src/ast/transition_constraints.rs

+    VecElem(Identifier, usize),
+    MatrixElem(Identifier, usize, usize),


I think we should avoid hard-coding the order of the indices. Instead, maybe we should make new shared atomic types like "VectorAccess" and "MatrixAccess" which can be used both here and in the boundary constraints.

Something like:

#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone)] pub struct VectorAccess { name: Identifier, idx: u64, } #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone)] pub struct MatrixAccess { name: Identifier, col_idx: u64, row_idx: u64, }

Also, if we're naming these like this then maybe we should change the name of "Var" to "Elem". It will be more consistent with these names, and they're no longer just variables now that they include named constants

Tbh, I'm not 100% convinced by the naming here, since a vector access may not be an element - it might be another vector. Do you have other ideas for this instead of VecElem and MatrixElem? Lookup, access, index, ... (I'm not in love with anything I've come up with)

I think "access" is the best of these, but I agree that it is not satisfying.

parser/src/ast/boundary_constraints.rs

parser/src/parser/tests/constants.rs

grjte · 2022-11-25T12:06:21Z

parser/src/parser/tests/boundary_constraints.rs

-                BoundaryExpr::PubInput(Identifier("a".to_string()), 0),
+                BoundaryExpr::VecElem(Identifier("a".to_string()), 0),


the name of this test should be updated

I went with adding a public_inputs section instead and kept the name same.

parser/src/parser/grammar.lalrpop

grjte · 2022-11-25T12:56:46Z

Should we enforce using CAPITALS for constant names? If yes, should we throw an error or show a warning to the user?

I'm in favor of this. It is kind of consistent with Rust. But also, I don't feel strongly about this.

I think we should probably do this. It's the convention for Python too, which means it's the convention for both languages we're referencing syntactically.

(This actually makes me wonder if we should really have a separate source section for these. Maybe they should just be declared at the top of the file uppercase, like the convention in Python and Rust. Having the lowercase source section "constants:" followed by the uppercase constants feels weird.)

Should we have any special characters for accessing constant values to ensure we know what the element is while building the AST instead of knowing after cross referencing it in the symbol table while building IR?

What would be an example of using special characters here? If it is prefixing every constant reference with a special character, I don't think we need it (especially if we use capital letters for constant names).

No, I don't think we should have special characters for accessing constant values.

tohrnii · 2022-11-25T16:17:12Z

(This actually makes me wonder if we should really have a separate source section for these. Maybe they should just be declared at the top of the file uppercase, like the convention in Python and Rust. Having the lowercase source section "constants:" followed by the uppercase constants feels weird.)

@grjte I agree with your point however since currently all of our code is organized into sections, I think it might also look a little weird to only have constants declared outside a section. Maybe we should only do this if we are planning to have some other part of the code outside of the current source section structure as well. What do you think?

tohrnii · 2022-12-01T07:35:52Z

Thank you @grjte for the review. Made suggested changes.

I've also added enforcing the use of capitalized names for constants in this PR. The error message and corresponding error enum variant name can be improved though but I can't think of a better one.

grjte

Thanks @tohrnii! This is a partial review of everything except the tests. I'll review the test changes separately.

parser/src/ast/mod.rs

parser/src/error.rs

parser/src/parser/grammar.lalrpop

tohrnii · 2022-12-02T16:31:50Z

@grjte I've changed VectorAccess to VecAccess and Matrix to MatAccess but haven't changed it for variants of ConstantType since then Scalar will probably need to be shortened as well. Please let me know if this change should be reverted for uniformity or if MatAccess doesn't look good.

grjte · 2022-12-02T17:44:35Z

@grjte I've changed VectorAccess to VecAccess and Matrix to MatAccess but haven't changed it for variants of ConstantType since then Scalar will probably need to be shortened as well. Please let me know if this change should be reverted for uniformity or if MatAccess doesn't look good.

Hmm yeah, I probably would revert for uniformity personally. Matrix is hard to abbreviate well

grjte

Thanks @tohrnii! Just a few small final things inline

parser/src/ast/transition_constraints.rs

parser/src/parser/grammar.lalrpop

parser/src/parser/tests/constants.rs

tohrnii · 2022-12-02T19:14:54Z

Hmm yeah, I probably would revert for uniformity personally. Matrix is hard to abbreviate well

Done.

Fumuran

Looks good! Thank you!

Al-Kindi-0

Looks great, thank you! I left only a question on testing.

Al-Kindi-0 · 2022-12-03T11:46:07Z

parser/src/lexer/tests/constants.rs

+}
+
+#[test]
+fn constants_access_inside_boundary_expr() {


Do we have some failing tests?

Thank you. I added a few tests with comments that should fail at the parser or IR level. I don't think there's anything specific to constants that would fail at the lexer level (please let me know if you think there are failure cases to add here). There are a lot of redundant tests for lexer and we should refactor these in the future and probably remove most of them.

grjte

Looks good! There's one final thing that should be adjusted, but I'm approving so I don't block things. Please make that adjustment before merging. Thanks for all your work on this!

grjte · 2022-12-03T17:50:39Z

parser/src/ast/mod.rs

+
+    /// Returns the name of the matrix.
+    pub fn name(&self) -> &str {
+        &self.name.0


This should also be &self.name.name() now that we have it

Ah sorry, I thought I changed both instances. Fixed.

tohrnii force-pushed the constants branch 2 times, most recently from c5772d7 to 82ca60f Compare November 4, 2022 14:55

tohrnii changed the title ~~feat(parser): add support for parsing of constants~~ Add support for parsing of constants Nov 4, 2022

tohrnii requested review from grjte and bobbinth November 4, 2022 14:56

tohrnii force-pushed the constants branch from 82ca60f to 26a024a Compare November 4, 2022 14:57

tohrnii marked this pull request as ready for review November 4, 2022 14:57

tohrnii force-pushed the constants branch 2 times, most recently from 8bc6d31 to 801dd1d Compare November 4, 2022 20:50

tohrnii marked this pull request as draft November 5, 2022 22:52

tohrnii removed request for bobbinth and grjte November 5, 2022 22:52

tohrnii marked this pull request as ready for review November 13, 2022 07:31

tohrnii requested review from grjte and bobbinth and removed request for grjte November 13, 2022 07:31

tohrnii force-pushed the constants branch from 2d10f4d to 82dddf9 Compare November 13, 2022 07:41

tohrnii changed the base branch from main to next November 13, 2022 19:18

tohrnii mentioned this pull request Nov 13, 2022

Add support for variables to parser #65

Merged

tohrnii requested review from Al-Kindi-0 and Fumuran November 21, 2022 17:07

tohrnii mentioned this pull request Nov 22, 2022

Tracking PR for v0.2 release #79

Merged

14 tasks

grjte removed the request for review from bobbinth November 25, 2022 10:42

grjte suggested changes Nov 25, 2022

View reviewed changes

tohrnii requested a review from grjte December 1, 2022 02:52

tohrnii force-pushed the constants branch from a7b2d75 to 1f00061 Compare December 1, 2022 07:30

grjte mentioned this pull request Dec 1, 2022

Tracking issue: Improved ergonomics for defining constraints in AirScript #77

Closed

tohrnii force-pushed the constants branch 2 times, most recently from 3a6e4ac to 837de82 Compare December 2, 2022 08:52

grjte suggested changes Dec 2, 2022

View reviewed changes

tohrnii force-pushed the constants branch 2 times, most recently from 71f68e1 to ad2da0e Compare December 2, 2022 16:25

grjte suggested changes Dec 2, 2022

View reviewed changes

parser/src/ast/transition_constraints.rs Outdated Show resolved Hide resolved

parser/src/parser/grammar.lalrpop Show resolved Hide resolved

parser/src/parser/tests/constants.rs Outdated Show resolved Hide resolved

parser/src/parser/tests/constants.rs Outdated Show resolved Hide resolved

tohrnii force-pushed the constants branch 5 times, most recently from 3c3c46d to ea68dcc Compare December 2, 2022 19:11

tohrnii requested a review from grjte December 2, 2022 19:14

tohrnii force-pushed the constants branch from ea68dcc to e79e892 Compare December 2, 2022 19:38

Fumuran approved these changes Dec 3, 2022

View reviewed changes

Al-Kindi-0 approved these changes Dec 3, 2022

View reviewed changes

tohrnii force-pushed the constants branch from e79e892 to 33181de Compare December 3, 2022 12:06

grjte approved these changes Dec 3, 2022

View reviewed changes

test: add more tests for edge cases for const parsing

c1b9bd0

tohrnii force-pushed the constants branch from 33181de to c1b9bd0 Compare December 3, 2022 21:23

tohrnii merged commit b78fb70 into 0xPolygonMiden:next Dec 3, 2022

tohrnii deleted the constants branch December 3, 2022 21:32

tohrnii restored the constants branch December 3, 2022 21:32

tohrnii mentioned this pull request Dec 8, 2022

Add support for declaring named constants (scalars, vectors, matrices) #60

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing of constants #63

Add support for parsing of constants #63

tohrnii commented Nov 4, 2022 •

edited

Loading

bobbinth commented Nov 15, 2022

tohrnii commented Nov 15, 2022 •

edited

Loading

grjte commented Nov 25, 2022

grjte left a comment

grjte Nov 25, 2022

grjte Nov 25, 2022 •

edited

Loading

Al-Kindi-0 Nov 25, 2022

grjte Nov 25, 2022

tohrnii Dec 1, 2022

grjte commented Nov 25, 2022 •

edited

Loading

tohrnii commented Nov 25, 2022

tohrnii commented Dec 1, 2022 •

edited

Loading

grjte left a comment

tohrnii commented Dec 2, 2022 •

edited

Loading

grjte commented Dec 2, 2022

grjte left a comment

tohrnii commented Dec 2, 2022 •

edited

Loading

Fumuran left a comment

Al-Kindi-0 left a comment

Al-Kindi-0 Dec 3, 2022

tohrnii Dec 3, 2022

grjte left a comment

grjte Dec 3, 2022

tohrnii Dec 3, 2022

		VecElem(Identifier, usize),
		MatrixElem(Identifier, usize, usize),

		BoundaryExpr::PubInput(Identifier("a".to_string()), 0),
		BoundaryExpr::VecElem(Identifier("a".to_string()), 0),

Add support for parsing of constants #63

Add support for parsing of constants #63

Conversation

tohrnii commented Nov 4, 2022 • edited Loading

bobbinth commented Nov 15, 2022

tohrnii commented Nov 15, 2022 • edited Loading

grjte commented Nov 25, 2022

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte commented Nov 25, 2022 • edited Loading

tohrnii commented Nov 25, 2022

tohrnii commented Dec 1, 2022 • edited Loading

grjte left a comment

Choose a reason for hiding this comment

tohrnii commented Dec 2, 2022 • edited Loading

grjte commented Dec 2, 2022

grjte left a comment

Choose a reason for hiding this comment

tohrnii commented Dec 2, 2022 • edited Loading

Fumuran left a comment

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tohrnii commented Nov 4, 2022 •

edited

Loading

tohrnii commented Nov 15, 2022 •

edited

Loading

grjte Nov 25, 2022 •

edited

Loading

grjte commented Nov 25, 2022 •

edited

Loading

tohrnii commented Dec 1, 2022 •

edited

Loading

tohrnii commented Dec 2, 2022 •

edited

Loading

tohrnii commented Dec 2, 2022 •

edited

Loading