Skip to content
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

check if symbol is defined before trying to access it #440

Closed
wants to merge 2 commits into from

Conversation

pfitzseb
Copy link
Member

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't these symbols wrapped into QuoteNodes ... ?

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #440 (1fb51ca) into master (8df3bef) will decrease coverage by 3.90%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
- Coverage   89.37%   85.46%   -3.91%     
==========================================
  Files          12       12              
  Lines        2268     2250      -18     
==========================================
- Hits         2027     1923     -104     
- Misses        241      327      +86     
Impacted Files Coverage Δ
src/interpret.jl 72.57% <100.00%> (-14.08%) ⬇️
src/builtins.jl 72.06% <0.00%> (-3.92%) ⬇️
src/construct.jl 88.19% <0.00%> (-2.57%) ⬇️
src/utils.jl 87.26% <0.00%> (-2.16%) ⬇️
src/types.jl 73.75% <0.00%> (-1.42%) ⬇️
src/packagedef.jl 87.50% <0.00%> (-1.08%) ⬇️
src/optimize.jl 96.84% <0.00%> (-1.06%) ⬇️
src/commands.jl 92.53% <0.00%> (-0.42%) ⬇️
src/breakpoints.jl 94.91% <0.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8df3bef...0406840. Read the comment docs.

timholy
timholy previously approved these changes Nov 19, 2020
@timholy
Copy link
Member

timholy commented Nov 21, 2020

Actually, this is more subtle than I initially thought. This essentially kicks the can down the road; suppose there is an object named bar but someone also uses :bar as an argument in the method. Is that supposed to refer to the object or is the symbol to be taken literally? In this case, the fix is to see if there is an object by that name, if so return it, otherwise assume that the symbol is to be taken literally. As @aviatesk says, in typical circumstances these are distinguished by wrapping them in a QuoteNode.

I almost wonder if we should delete those Symbol lookup methods (that breaks a lot of tests, but maybe they can all be addressed individually). They might essentially correspond to cases where we fail to

timholy added a commit that referenced this pull request Nov 21, 2020
`evaluate_call_recurse!` calls `maybe_evaluate_builtin`, and if
that doesn't return a `Some{Any}`, it uses the output as the
new `call_expr`. The next thing it does is look up the args.
Consequently, to avoid double-lookup, our expansion of `invoke` should
return the non-looked-up arguments.

Fixes #441
Closes #440
@timholy timholy dismissed their stale review November 21, 2020 11:43

Better fix in #442

@timholy timholy closed this in #442 Nov 21, 2020
timholy added a commit that referenced this pull request Nov 21, 2020
`evaluate_call_recurse!` calls `maybe_evaluate_builtin`, and if
that doesn't return a `Some{Any}`, it uses the output as the
new `call_expr`. The next thing it does is look up the args.
Consequently, to avoid double-lookup, our expansion of `invoke` should
return the non-looked-up arguments.

Fixes #441
Closes #440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants