-
Notifications
You must be signed in to change notification settings - Fork 11
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
MassaLabs: Allow named constants range notation #355
MassaLabs: Allow named constants range notation #355
Conversation
@Leo-Besancon I needed to get my head back in the gritty details a bit, so I ended up prototyping a solution for this in my My implementation is based on the assumption that named constants can be replaced with the actual values very early during compilation. This is because we have no reason to preserve the use of the name, and everywhere else relies on being able to access the actual value (as you've discovered). So this is exactly what I've implemented in my branch - after parsing, but during the initial visit of a module during semantic analysis, any time an expression is evaluated, we are first resolving all symbols of that expression, so we take that opportunity to ensure that references to constant identifiers are valid. Then, in the case of ranges, the next thing we do is visit the range bounds, and after resolving the symbol, rewrite the bound with the actual constant that was referenced, or raise an error if the reference was invalid (either the symbol doesn't exist, isn't a constant, or the value is not a valid constant scalar value). Then in all subsequent parts of the pipeline, it is safe to assume that range bounds are always constant, and panic if that assumption is violated. The solution is a bit hacky, but largely because we are using the AST as an IR as well, so we end up having to encode a lot of information in the same data structure, which lends to it feeling hacky. Once we have a proper IR in place, the IR can assume all references to named constants are reified and things become a lot cleaner. Let me know if you have further questions, or run into any issues with your implementation! |
Hello, perfect, thank you for answering my question in such a detailed manner! It helps clear things up and aligns with what I thought was the more realistic approach. |
This makes it possible to reference named constants in a range, or slice indexing operation. NOTE: The named constants are rewritten with the values they refer to early during compilation, so in most of the pipeline you will never be able to observe anything but constant range bounds. However, rather than complicate the AST further by making that distinction explicit, we reuse a single type for all ranges, which means we have a lot of apparently-fallible operations in places where they are actually infallible, but this seems like a decent compromise.
Fix example.air after 0xPolygonMiden#353
20e2ea4
to
0a7796c
Compare
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.
LGTM! Definitely a handy improvement!
@bobbinth Let me know if you still plan to review this, otherwise I'll try to get this merged tomorrow |
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 good! Thank you! Though this is a pretty shallow review from me.
Hello @bitwalker did you want to merge this one? :) |
@Leo-Besancon Sorry about that, forgot to follow up on this, thanks for the ping! |
Draft PR for #266
I wanted some input for the design there @bitwalker.
The main issue is that AirScript aggregate types contain the size in their definitions, and so we don't know the type of ranges containing named constants until we resolve their value.
Moreover, the semantic analysis pass expects to know the various sizes.
For example, while checking list comprehension, we check that all iterator types are the same. Some code use placeholder (
u32::MAX as usize
) when the size is unknown, (I think this is currently unused), but I'm thinking of two options:we couldn't do during semantic analysis without knowing the full type). I tried to do this at first but it seemed a bit ugly
What are your thoughts? Is there anything else you think I'm missing?
TODO: