-
-
Notifications
You must be signed in to change notification settings - Fork 114
Start analyzing & evaluating meta
#576
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: main
Are you sure you want to change the base?
Start analyzing & evaluating meta
#576
Conversation
fccfb62 to
af164e0
Compare
|
Currently the following doesn't work: But before I dive deeper I thought best align on where to store the analysis results for |
jeaye
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.
Good start, Shantanu. Your overall unwrap usage is lacking a lot of necessary discipline. The testing strategy will need some more detailed planning, too, since this opens up a lot of doors. Finally, IR gen has not been updated, so none of this will work when wrapped in a function or AOT compiled.
| (assert (= one 'one)) | ||
| (assert (= (:doc (meta #'one)) nil)) | ||
|
|
||
| ; Analysis with global variables |
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.
This is not how new tests should be added. Each test should have a dedicated test file, describing what it's testing. Also, we now have possibilities for extra errors, which come from analyzing meta. That needs to be tested.
| { | ||
| auto var(__rt_ctx->intern_var(expr->name).expect_ok()); | ||
| var->meta = expr->name->meta; | ||
| var->meta = some(eval(expr->meta.unwrap())); |
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.
This is lazy and not safe (because we don't know that meta contains a value). Look at line 281 for not only the variable you should use, but how to safely grab this value.
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've chosen to wrap the unwrap in an is_some block so that I won't even bother calling eval when there is no meta available.
|
|
||
| ; Analysis with global variables | ||
| (def foo 42) | ||
| (def ^{:pp #(println foo)} bar 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.
This will fail if you wrap it in a function or let*, since IR gen has not been updated.
0074bcb to
a3acb47
Compare
6b6b2b2 to
87cc531
Compare
Description
We should begin analyzing the metadata as well. At present, we store the metadata source directly without proper analysis or evaluation, resulting in the following code executing despite being semantically incorrect:
There are two issues being highlighted here:
foosymbol is not in scope during the definition of thebarvar, jank does not emit any analysis failure warnings.:ppkey in the metadata of thebarvar contains the source code instead of a compiled function, preventing us from invoking the function.Goal
Within the scope of this PR I wish to start analyzing the metadata such that the above cases behave as would be expected:
Fixes
Resolves #197.