-
Notifications
You must be signed in to change notification settings - Fork 19
Implement first version of the custom sigil functionality #158
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
base: master
Are you sure you want to change the base?
Implement first version of the custom sigil functionality #158
Conversation
|
Hey. This is neat. I may take longer to review than usual. I am on a business trip in Europe and jetlagged right now since I just landed today from the US. I should get to it next week though, once I'm back home. I'll take a look but would rather not merge just yet on account of my limited brain power this week :) |
egranata
left a comment
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.
Some feedback - as I mentioned earlier, I'd rather wait to merge until I am a bit more available, but some early thinks to think about :)
parser-lib/src/grammar/grammar.pest
Outdated
| postfix_term_object_write = { "{" ~ postfix_term_write_list ~ "}" } | ||
| postfix_term_enum_case = { "::" ~ identifier ~ ("(" ~ expression ~ ")")? } | ||
| postfix_term_sigil = @{ "_" ~ ASCII_ALPHA+ } | ||
| postfix_term = { |
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.
If I understand correctly, doing this via a postfix means that - in theory - you can apply sigils to function calls, attribute reads, ... - instead of just a literal? Is that intentional? Do you have use cases in mind?
| PostfixValue::Sigil(base, sigil, loc) => { | ||
| base.emit_read(params)?; | ||
|
|
||
| let internal_name = format!("__sigil_{sigil}"); |
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.
IIRC, __ is a reserved prefix, which means you cannot use it as an identifier
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.
wdym? because it seems like that the grammar forbids __ prefixed identifiers in user code, but compiler and vm can create this names (for example __repl_chunk)
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.
Right, right, the VM can do whatever. But it does mean that if you have sigil-A that wants to reuse the code from sigil-B, you can't do it directly, without defining a helper function in the middle. I ran into the same problem when I did the whole operator overloading thing. Initially, I had __op_this_op_that and it meant that if you had operator + and reverse operator +, you couldn't just have one call the other because user code could not use the __ identifier. I fixed it by making the operator functions be _op (single _)
Maybe this is less of a problem for sigils?
| vm: &mut crate::vm::VirtualMachine, | ||
| ) -> crate::vm::ExecutionResult<RunloopExit> { | ||
| let sigil_name_val = cur_frame.stack.pop(); | ||
| let function = cur_frame.stack.pop(); |
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.
What happens if I pass a non-function thing here? In theory, I should be able to pass:
- a closure
- a function
- an object with
operator calldefined
and it should all work, but if I say
register_sigil(foo, 3) then it should fail? Or do you think you should be able to pass anything and then it gets validated at runtime?
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.
Also, what happens if I redefine a sigil? Is that allowed? Should it fail?
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.
it makes more sense to catch errors early, and also to not redefine them. Changed
|
I had one thought - say that I have does A few options I can think of, in no particular order of preference (yet):
Anything I am missing here? Either in terms of possible resolutions or maybe I am wrong about this problem entirely? |
|
@enricobolzonello friendly ping if you're still interested in merging this - if not, let me know & I can keep working on it |
|
hey, sorry for the inactivity, got stuck with work. I'm going to work on it this weekend :) |
no worries at all - just checking what you wanted done with this PR :) |
egranata
left a comment
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.
Few questions, but getting closer. Thanks for hacking at this!
| register_sigil("sigil_name", function_to_call); | ||
| ``` | ||
|
|
||
| Complete example: |
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.
am I missing something, or does this code never call register_sigil?
| fn from_parse_tree(p: pest::iterators::Pair<'_, Rule>, source: &SourceBuffer) -> Self { | ||
| assert!(p.as_rule() == Rule::postfix_term_sigil); | ||
| let loc = From::from(&p.as_span()); | ||
| let sigil = p.as_str().strip_prefix("@").unwrap().to_string(); |
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.
maybe expect instead of unwrap so it crashes with some kind of readable-ish error?
| @@ -0,0 +1,11 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
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.
suggest adding tests for a few things:
- free function
- method (you do that)
- closure
- not a callable thing at all
and checking what each does?
| let sigil_name = sigil_name_val.prettyprint(cur_frame, vm); | ||
|
|
||
| let internal_name = format!("__sigil_{sigil_name}"); | ||
| if function.as_function().is_some() |
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.
not your problem, but I should probably centralize this code somewhere; once we merge your PR we have 3 places that do this exact resolution
| .is_ok() | ||
| { | ||
| if vm.sigil_registry.read(&internal_name).is_some() { | ||
| return Err(VmErrorReason::OperationFailed(format!( |
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 am a bit torn on this - on the one hand, most things in Aria can freely be redefined, e.g.
〉struct Foo {
::: type func new() = alloc(This);
::: func bar() = 42;
::: }
〉val f = Foo.new();
〉f.bar();
42
〉extension Foo {
::: func bar() = 44;
::: }
〉f.bar();
44
on the other hand, maybe if your literals break horribly that's a step too far?
| } | ||
|
|
||
| pub(super) fn insert_builtins(builtins: &mut VmBuiltins) { | ||
| builtins.insert_builtin::<RegisterSigil>(); |
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.
should there be a way to read all registered sigils? unregister one? wdyt?
Part of #7. In the first version, it allows defining custom sigils, but only with alphabetic characters for now (using identifier created ambiguity in the grammar). Basically, sigils are syntactic sugar around functions: they are stored and called just like functions, but are prefixed with _sigil. However it creates the issue that _sigil is effectively a reserved keyword.
I'm not particularly fond of the use of a separate global register for sigils, but I couldn't come up with anything better: directly translating to functions means that probably using sigils from other modules would be a pain, while registering them as built-ins didn't feel right. I'm open to other possibilities that I have missed.
Next on the todo list is to implement some sigils for the stdlib