Refactor asm ast #1736

Schaeff · 2024-08-30T12:24:59Z

No description provided.

…ed-and-asm-analysis-ast

chriseth · 2024-09-02T10:54:58Z

airgen/src/lib.rs

+                    .filter(|s| {
+                        matches!(
+                            s,
+                            PilStatement::EnumDeclaration(..) | PilStatement::LetStatement(..)


I already envision spending quite some time trying to figure out why something is missing from the output. Is there a safer way to do this check so that we add variants here if the code is extended?

using an exhaustive match instead

asm-to-pil/src/vm_to_constrained.rs

chriseth · 2024-09-02T11:02:37Z

ast/src/asm_analysis/mod.rs

        })
    }
+
+    pub fn get_machine(&self, ty: &AbsoluteSymbolPath) -> Option<&Machine> {
+        let mut path = ty.clone();


I the clone needed here?

Yes since modules are indexed by path and machines by name. To avoid this clone, we'd need slices of AbsoluteSymbolPath which is not implemented in this PR.

chriseth · 2024-09-02T11:04:01Z

ast/src/object/mod.rs

@@ -41,7 +41,7 @@ pub struct PILGraph {
    pub main: Machine,
    pub entry_points: Vec<Operation>,
    pub objects: BTreeMap<Location, Object>,
-    pub definitions: BTreeMap<AbsoluteSymbolPath, TypeOrExpression>,
+    pub definitions: BTreeMap<AbsoluteSymbolPath, Vec<PilStatement>>,


Is the key here the module name?

yes, comment added

Maybe we should also rename this to statements?

chriseth · 2024-09-02T11:06:01Z

ast/src/parsed/asm.rs

-            ModuleStatement::TraitImplementation(_) => None,
+            ModuleStatement::SymbolDefinition(d) => Box::new(once(&d.name)),
+            ModuleStatement::PilStatement(s) => {
+                Box::new(s.symbol_definition_names().map(|(name, _)| name))


I think we should call symbol_definition_names_and_contained here, but I'm not sure. .._and_contained also returns the variants in an enum for example.

So... if we have

enum Foo { Bar }

Then the defined names should be ["Foo", "Bar"]? Or ["Foo", "Foo::Bar"]?

[("Foo", None), ("Foo", Some("Bar"))]

I mean, what should this function return? Currently it returns all names defined in a given statement, as strings. Should the signature then change?

At least before your PR this was essentially only used by the call to local_names() inside check_machine to see if there is a name clash, so I think it's not needed to return the nested names.

It's used only in the importer to prevent duplicates, and to detect the standard library. I think we do not need the nested names for that. Then the current code is correct?

chriseth · 2024-09-02T11:06:51Z

ast/src/parsed/asm.rs

@@ -97,7 +81,7 @@ pub enum SymbolValueRef<'a> {
    /// A module definition
    Module(ModuleRef<'a>),
    /// A generic symbol / function.
-    Expression(&'a TypedExpression),
+    Expression(&'a Option<Expression>, &'a Option<TypeScheme<Expression>>),


Why not keep TypedExpression? Lifetime issues?

Because it's constructed from a reference over PilStatement::LetStatement which is

LetStatement( SourceRef, String, Option<TypeScheme<Expression>>, Option<Expression>, ),

My understanding is that SymbolValueRef::Expression also covers statements like let a: Type; which seem to be allowed. TypedExpression would not cover that.

chriseth · 2024-09-02T11:11:14Z

importer/src/path_canonicalizer.rs

+            }
+        }
+        PilStatement::TraitImplementation(_, trait_impl) => {
+            for f in &mut trait_impl.functions {


Should also call canonicalize_inside_type_scheme for trait_impl.type_scheme.

Extracted to https://github.com/powdr-labs/powdr/pull/1750/files because I'm not sure

Following discussion, removed the commit and instead used generics on the existing data structures

#1736 (comment)

chriseth · 2024-09-04T15:25:55Z

airgen/src/lib.rs

+                        PilStatement::Include(..) => false,
+                        PilStatement::Namespace(..) => false,
+                        PilStatement::PolynomialDefinition(..) => false,
+                        PilStatement::PublicDeclaration(..) => false,
+                        PilStatement::PolynomialConstantDeclaration(..) => false,
+                        PilStatement::PolynomialConstantDefinition(..) => false,
+                        PilStatement::PolynomialCommitDeclaration(..) => false,
+                        PilStatement::PlookupIdentity(..) => false,
+                        PilStatement::PermutationIdentity(..) => false,
+                        PilStatement::ConnectIdentity(..) => false,
+                        PilStatement::TraitImplementation(..) => false,
+                        PilStatement::TraitDeclaration(..) => false,
+                        PilStatement::Expression(..) => false,


Suggested change

PilStatement::Include(..) => false,

PilStatement::Namespace(..) => false,

PilStatement::PolynomialDefinition(..) => false,

PilStatement::PublicDeclaration(..) => false,

PilStatement::PolynomialConstantDeclaration(..) => false,

PilStatement::PolynomialConstantDefinition(..) => false,

PilStatement::PolynomialCommitDeclaration(..) => false,

PilStatement::PlookupIdentity(..) => false,

PilStatement::PermutationIdentity(..) => false,

PilStatement::ConnectIdentity(..) => false,

PilStatement::TraitImplementation(..) => false,

PilStatement::TraitDeclaration(..) => false,

PilStatement::Expression(..) => false,

PilStatement::Include(..) |

PilStatement::Namespace(..) |

PilStatement::PolynomialDefinition(..) |

PilStatement::PublicDeclaration(..) |

PilStatement::PolynomialConstantDeclaration(..) |

PilStatement::PolynomialConstantDefinition(..) |

PilStatement::PolynomialCommitDeclaration(..) |

PilStatement::PlookupIdentity(..) |

PilStatement::PermutationIdentity(..) |

PilStatement::ConnectIdentity(..) |

PilStatement::TraitImplementation(..) |

PilStatement::TraitDeclaration(..) |

PilStatement::Expression(..) => false,

importer/src/path_canonicalizer.rs

chriseth · 2024-09-24T11:17:12Z

ast/src/parsed/types.rs

+
+/// A trait to operate the possible types for the array type lengths
+pub trait ArrayLength: std::fmt::Display + std::fmt::Debug {
+    fn try_to_expression_mut(&mut self) -> Option<&mut Expression>;


try_as_expression_mut?

chriseth · 2024-09-24T11:18:04Z

ast/src/parsed/types.rs

+}
+
+/// A trait to operate the possible types for the array type lengths
+pub trait ArrayLength: std::fmt::Display + std::fmt::Debug {


ExpressionInArrayLength?

wip

236c3ad

Schaeff mentioned this pull request Aug 30, 2024

Better parsed AST #1725

Closed

Schaeff added 6 commits August 30, 2024 17:58

clean

cd850d1

fix test, better module api

a317a4d

fix test

b2b154d

Merge branch 'main' of github.com:powdr-labs/powdr into refactor-pars…

4172c70

…ed-and-asm-analysis-ast

fix more tests

008fb73

clean

578b1e6

leonardoalt requested a review from chriseth September 2, 2024 08:23

clean

95089bf

chriseth reviewed Sep 2, 2024

View reviewed changes

asm-to-pil/src/vm_to_constrained.rs Show resolved Hide resolved

chriseth reviewed Sep 2, 2024

View reviewed changes

Schaeff added 3 commits September 2, 2024 17:11

add comment

19ab9b8

pull main

e1f8a22

address review comments

1928b49

Schaeff mentioned this pull request Sep 3, 2024

Use expression as type param in trait implementation #1750

Merged

Schaeff added a commit that referenced this pull request Sep 4, 2024

Use expression as type param in trait implementation (#1750)

5a50e2e

#1736 (comment)

chriseth reviewed Sep 4, 2024

View reviewed changes

Schaeff force-pushed the refactor-parsed-and-asm-analysis-ast branch from 5a50e2e to af73301 Compare September 6, 2024 23:14

Schaeff commented Sep 6, 2024

View reviewed changes

importer/src/path_canonicalizer.rs Outdated Show resolved Hide resolved

Schaeff force-pushed the refactor-parsed-and-asm-analysis-ast branch from af73301 to 1928b49 Compare September 13, 2024 15:44

pull main

37d4bd2

Schaeff commented Sep 14, 2024

View reviewed changes

importer/src/path_canonicalizer.rs Outdated Show resolved Hide resolved

canonicalize trait implementations

24550f5

Schaeff force-pushed the refactor-parsed-and-asm-analysis-ast branch from c04d0d7 to 24550f5 Compare September 15, 2024 22:12

typo

7285fbc

Schaeff requested a review from chriseth September 15, 2024 22:34

Schaeff added 2 commits September 23, 2024 16:24

pull main

138779c

pull main

49d1dd2

chriseth reviewed Sep 24, 2024

View reviewed changes

review comments

352321a

chriseth approved these changes Sep 24, 2024

View reviewed changes

chriseth enabled auto-merge September 24, 2024 12:27

chriseth added this pull request to the merge queue Sep 24, 2024

Merged via the queue into main with commit 64e7bf1 Sep 24, 2024
14 checks passed

chriseth deleted the refactor-parsed-and-asm-analysis-ast branch September 24, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor asm ast #1736

Refactor asm ast #1736

Schaeff commented Aug 30, 2024

chriseth Sep 2, 2024

Schaeff Sep 4, 2024

chriseth Sep 2, 2024

Schaeff Sep 3, 2024

chriseth Sep 2, 2024

Schaeff Sep 2, 2024

chriseth Sep 2, 2024

Schaeff Sep 4, 2024

chriseth Sep 2, 2024

Schaeff Sep 3, 2024 •

edited

Loading

chriseth Sep 4, 2024

Schaeff Sep 5, 2024

chriseth Sep 5, 2024

Schaeff Sep 6, 2024

chriseth Sep 2, 2024

Schaeff Sep 3, 2024

chriseth Sep 2, 2024

Schaeff Sep 3, 2024

Schaeff Sep 4, 2024

Schaeff Sep 6, 2024

chriseth Sep 4, 2024

chriseth Sep 24, 2024

Schaeff Sep 24, 2024

chriseth Sep 24, 2024

Schaeff Sep 24, 2024

Refactor asm ast #1736

Refactor asm ast #1736

Conversation

Schaeff commented Aug 30, 2024

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

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

Choose a reason for hiding this comment

Schaeff Sep 3, 2024 • 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

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

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

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

Schaeff Sep 3, 2024 •

edited

Loading