Salsa-based analyzer - #468
Conversation
748e4f0 to
ae289b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #468 +/- ##
==========================================
- Coverage 87.69% 87.25% -0.45%
==========================================
Files 80 86 +6
Lines 5276 5515 +239
==========================================
+ Hits 4627 4812 +185
- Misses 649 703 +54
Continue to review full report at Codecov.
|
g-r-a-n-t
left a comment
g-r-a-n-t
left a comment
There was a problem hiding this comment.
looks awesome 🎊!
I'm going to spend some more time going over this tomorrow, as there is alot to digest.
| .into_node() | ||
| } | ||
|
|
||
| pub fn type_desc1(context: &mut ModuleContext, desc: Node<TypeDesc>, typ: &Type) -> Node<TypeDesc> { |
There was a problem hiding this comment.
a doc comment would be nice here
There was a problem hiding this comment.
Whoops, that's just a refactor-in-progress that wasn't finished or subsequently cleaned up. There should only be one type_desc fn here. I'll fix.
| @@ -0,0 +1,28 @@ | |||
| type Posts = Map<PostId, PostBody> | |||
| @@ -44,27 +40,22 @@ fn dispatch_arm(attributes: FunctionAttributes) -> yul::Case { | |||
| // If the function returns a unit value, we call the function and return | |||
| // nothing. Otherwise, we encode the value and return it. | |||
| let call_and_maybe_encode_return = { | |||
| let name = identifier! { (format!("$${}", attributes.name)) }; | |||
| let name = identifier! { (format!("$${}", name)) }; | |||
There was a problem hiding this comment.
I realize you were only passing thru.. but we should be using names::func_name here
I can push a commit for this
| contract: ContractId, | ||
| ) -> Vec<yul::Statement> { | ||
| // TODO: db should provide easier access to these things. | ||
| // Eg. contract.public_functions(db) |
There was a problem hiding this comment.
we might want the following methods:
init_function- returns an optional functionfunctions- returns all user defined functions, except for__init__public_functions- returns all public functions, except for__init__
this would sharpen up the places where we're checking the name of functions against __init__
it also led me to wonder whether or not the following contract would compile
contract Foo:
pub fn __init__():
pass
fn bar():
self.__init__()
it does, but I dont think it should. my thinking is that __init__ should not be callable, especially if we have special behavior like const initialization in them
There was a problem hiding this comment.
Yeah, I fully agree. I was thinking the same thing as I was querying the functions map for __init__, but I was desperate to just get stuff working and left it alone. I'll clean it up now though.
There was a problem hiding this comment.
@g-r-a-n-t see the latest commit. Calling __init__ on self or another value of type contract is now disallowed (because it's not in the functions/public_functions maps, but also I added a specific check for it so the error message could be better). I didn't touch Foo.__init__; that's already an error and the nearby code would require some reshuffling to add a helpful note about create/create2, which I was too tired to do it (added a // TODO: though).
I also added an error if one tries to call a non-pub fn on a (non-self) contract.
g-r-a-n-t
left a comment
g-r-a-n-t
left a comment
There was a problem hiding this comment.
Just left a few more comments related to the struct changes.
|
LGTM |
…ns on external contracts
3 participants
What was wrong?
The analyzer was implemented as a single pass through the AST, which meant that it wasn't possible to refer to a type defined later in the file or define recursive types.
How was it fixed?
The analyzer now uses https://crates.io/crates/salsa, and thus a query-based style. Traversal of function bodies is mostly unchanged, but the analysis of everything else (the so-called "items": modules, structs, contracts, type aliases, function definitions) now happens via queries defined in
analyzer/src/db/queries/. These items are "interned", and are referred to by ids (defined inanalyzer/src/namespace/items.rs). The id types have convenience functions to run queries; egcurrent_module.resolve_type(db, "Foo")orcontract.field(db, "accounts").typ(db).This required refactoring of the stages that use the analyzer results, so the lowering, abi, and yulgen stages now use a
&dyn AnalyzerDbto get the information they need.The
StructandContracttypes no longer include their fields (to allow for recursive types); you need a db to get the fields and their types. To avoid passing the db around absolutely everywhere, I changed some things that I otherwise would have preferred to not touch. So if you're wondering why I made some weird change, it was probably because of the lack of fields in theStructtype. Eg the abi dispatch generator code now takesAbiTypes instead ofTypes, andfn abi_componentsisn't part of theJsonAbitrait anymore. (I initially implementedabi_componentsas a query on an AbiDb, but talked myself out of that for now (along with the FilesDb and ParserDb that existed briefly)).I think a lot of this could be refined, especially if/when we start using salsa in other stages. Some of the code feels a bit awkward.
To-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history