-
Notifications
You must be signed in to change notification settings - Fork 197
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
Salsa-based analyzer #468
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome 🎊!
I'm going to spend some more time going over this tomorrow, as there is alot to digest.
crates/lowering/src/mappers/types.rs
Outdated
.into_node() | ||
} | ||
|
||
pub fn type_desc1(context: &mut ModuleContext, desc: Node<TypeDesc>, typ: &Type) -> Node<TypeDesc> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a doc comment would be nice here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
crates/test-files/fixtures/compile_errors/init_wrong_return_type.fe
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,28 @@ | |||
type Posts = Map<PostId, PostBody> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a few more comments related to the struct changes.
LGTM |
…ns on external contracts
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 AnalyzerDb
to get the information they need.The
Struct
andContract
types 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 theStruct
type. Eg the abi dispatch generator code now takesAbiType
s instead ofTypes
, andfn abi_components
isn't part of theJsonAbi
trait anymore. (I initially implementedabi_components
as 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