Skip to content

Conversation

@IanButterworth
Copy link
Member

Fixes #43446

.. by not looking beyond an @eval when collecting a list of modules that are going to be loaded for the missing package install prompt.

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.

Disabling the feature for eval expressions sound reasonable to me.

end
end
for arg in ast.args
arg == Symbol("@eval") && break # don't search beyond an `@eval`
Copy link
Member

Choose a reason for hiding this comment

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

The same kind of situation will also happen for Core.eval(xxx, ...) as well as xxx.eval(...).

Copy link
Member Author

Choose a reason for hiding this comment

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

@simeonschaub's suggestion below seems clean so I've gone for that

@simeonschaub
Copy link
Member

Instead of recursing into any expression, could we not just recurse into block expressions? I don't think it's legal for import and using statements to occur anywhere else.

@IanButterworth IanButterworth force-pushed the ib/pkg_add_prompt_dont_eval branch from 0ec75fe to 6ba8680 Compare December 31, 2021 05:22
@IanButterworth IanButterworth changed the title Missing package add prompt: Don't search for modules beyond an @eval Missing package add prompt: Restrict which exprs to search in Dec 31, 2021
@IanButterworth IanButterworth merged commit 8f192bd into JuliaLang:master Jan 1, 2022
@IanButterworth IanButterworth deleted the ib/pkg_add_prompt_dont_eval branch January 1, 2022 23:02
KristofferC pushed a commit that referenced this pull request Jan 5, 2022
@KristofferC KristofferC mentioned this pull request Jan 5, 2022
23 tasks
end
for arg in ast.args
arg isa Expr && modules_to_be_loaded(arg, mods)
if arg isa Expr && arg.head in [:block, :if, :using, :import]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if arg isa Expr && arg.head in [:block, :if, :using, :import]
if arg isa Expr && arg.head in (:block, :if, :using, :import)

The [] form allocates the same tuple as above, then splats it to a new array, on every iteration of the loop.

Copy link
Member

Choose a reason for hiding this comment

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

Or even more concisely:

Suggested change
if arg isa Expr && arg.head in [:block, :if, :using, :import]
if isexpr(arg, (:block, :if, :using, :import))

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, thanks #43708

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.

using xxx badly interacts with evaled expressions

5 participants