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

Missing package add prompt: Restrict which exprs to search in #43457

Merged

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.

@@ -194,6 +194,7 @@ function modules_to_be_loaded(ast::Expr, mods::Vector{Symbol} = Symbol[])
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 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
@@ -194,7 +194,9 @@ function modules_to_be_loaded(ast::Expr, mods::Vector{Symbol} = Symbol[])
end
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