-
Notifications
You must be signed in to change notification settings - Fork 323
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
Properly expose stacktraces and related data to user code #3271
Conversation
Test.specify "should capture traces correctly" <| | ||
modname = Meta.Constructor (Meta.meta here . constructor) . name | ||
stack = My_Type.foo | ||
names = [modname + ".bar", modname + ".baz", "Number.foo", modname + ".foo", "My_Type.foo"] |
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 was thinking if the names should be qualified.
Does it capture the name, as it was resolved at the get_stack_trace
call point? Or are the module names simply omitted?
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.
Qualified names are too noisy most of the time. But there's nothing stopping us from exposing both if we really wanted to (with the caveat that they will be missing in polyglot calls).
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, but the 'shadow definitions' with documentation for added functions are missing from Builtins.enso
.
/** Wrapper for exposing sources to Enso. Delegates to original methods with no behavior changes. */ | ||
public class EnsoSource { | ||
private final Source source; |
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.
Is there any deeper reason for which we want to have such a wrapper instead of exposing Source
directly?
Or is this just a facade so that if the API changes in the future, we can avoid having to update the Enso-side?
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.
Nah it's a dirty hack, you actually cannot expose source over polyglot, truffle will block you. This should actually be a builtin in object, once we finally get such a concept neatly exposed (which may have to happen soon, seeing as extension methods will be removed).
scope.registerMethod( | ||
polyglot, "get_executable_name", GetExecutableNameMethodGen.makeFunction(language)); | ||
scope.registerMethod( | ||
polyglot, "get_source_location", GetSourceLocationMethodGen.makeFunction(language)); | ||
scope.registerMethod( | ||
polyglot, "has_source_location", HasSourceLocationMethodGen.makeFunction(language)); |
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.
The corresponding shadow definitions in Builtins.enso
for these three and for Runtime.primitive_get_stack_trace
seem to be missing.
It is also not completely clear to me what does it mean to get a source location from a polyglot object. Does every object have it? Or does this only apply to stack trace elements? From the usage it seems like it is the latter, but it's unclear as it's not described. Also, if it is the latter, what is the reason that these methods must be exposed as methods of the Polyglot
module, can't we just use some getters of the stack trace elements (I assume we probably can't but would love to know why)?
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.
Does every object have it? Or does this only apply to stack trace elements?
Ok, by reading Truffle docs I answered this question.
Still, the docs for the builtins would be useful
Pull Request Description
Introduces an API to peek the current stack trace and programatically work with source locations.
Important Notes
This may be used in the future for exception handling. Currently just the bare facilities with the intent to use them with warnings.
Checklist
Please include the following checklist in your PR: