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

replace current_module() with @__MODULE__ #22064

Merged
merged 1 commit into from
Jun 6, 2017
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 25, 2017

Passes macros a __module__ argument (like the new __source__ argument from #21746), allowing them to inspect and configure themselves based on their evaluation context.

This removes current_module dependence from include:
Instead, we use the same trick as for eval (they are basically the same function after all) to create a module-relative function in each non-bare module.

fixes #20487

"""
@isdefined s -> Bool

Get whether symbol `s` is defined in the current scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Get whether" sounds a bit strange, that wording isn't used in any other docstrings

@@ -524,6 +523,11 @@ JL_CALLABLE(jl_f_isdefined)
}
if (nargs != 2) {
JL_NARGS(isdefined, 1, 1);
jl_depwarn("`isdefined(:symbol)` is deprecated, "
Copy link
Contributor

Choose a reason for hiding this comment

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

add a note in deprecated.jl reminding to delete this when the other deprecations from this PR are removed

Copy link
Contributor

Choose a reason for hiding this comment

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

"support machinery in src for current_module" is not very clear what it's referring to

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lot of it scattered everywhere. Do you really need me to write you a sed script

Copy link
Contributor

Choose a reason for hiding this comment

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

again, not a helpful attitude. enough information so that it can be cleaned up completely would be relevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

delete every line containing the deprecated method:

grep -RI current_module . | wc -l
62

and then refactor the eval methods to reflect the fact that this information no longer needs to be maintained

@StefanKarpinski
Copy link
Member

Can this info be stuffed into the __source__ type? It seems better to have one magic argument than >1 magic argument.

@vtjnash
Copy link
Member Author

vtjnash commented May 26, 2017

Of course it's possible to make a new type that encapsulates both, but I'm not certain that's entirely desirable. I don't currently anticipate this growing beyond these 2 arguments. Although we might eventually decide to introduce other names for accessing other contextual information for debugging such as __function__::Symbol.

@StefanKarpinski
Copy link
Member

I don't currently anticipate this growing beyond these 2 arguments.

Famous last words. Is it time to officially reserve names that start and end with double underscores yet? Or are just going to continue to hope that security-through-sufficient-number-of-underscores works in the future?

@vtjnash
Copy link
Member Author

vtjnash commented May 26, 2017

Only in macro arguments 😛 Are you going to reopen #18249?

@StefanKarpinski
Copy link
Member

Wasn't well received last time, so 🤷‍♂️ .

@tkelman
Copy link
Contributor

tkelman commented May 27, 2017

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@tkelman
Copy link
Contributor

tkelman commented May 27, 2017

Is the reason this can't be __source__.module that __source__ is about the definition site whereas __module__ is about the call site?

return binding_module(_current_module(), s)
end
@noinline function expand(x::ANY)
depwarn("expand(x) is deprecated, use `expand(__module__, x)` instead.", :expand)
Copy link
Contributor

Choose a reason for hiding this comment

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

using __module__ is only relevant syntax when this is used inside a macro definition, right? for interactive repl usage wouldn't @__MODULE__ be the more useful way to call it? or write this as expand(module, x) and let the user determine where they want to get the module argument from

Copy link
Member Author

Choose a reason for hiding this comment

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

For the REPL, you can just type Main. There's nothing particularly special about the name __module__ that makes it any different from module. I thought it was nicely reminiscent of the macro argument name (allowing easier search), and also provided underlining of the change (highlighting word diff).

Copy link
Contributor

Choose a reason for hiding this comment

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

we normally only put underscores around something when it's special, the deprecation replacement messages are more in line with names we would use for a docstring


Print information about exported global variables in a module, optionally restricted to those matching `pattern`.

The memory consumption estimate is an approximate lower bound on the size of the internal structure of the object.
"""
function whos(io::IO=STDOUT, m::Module=current_module(), pattern::Regex=r"")
function whos(io::IO=STDOUT, m::Module=Main, pattern::Regex=r"")
Copy link
Contributor

Choose a reason for hiding this comment

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

the behavior change here is worth noting more visibly for anyone who may be calling this programmatically inside non-main modules

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not actually a behavioral change in that case (both the old and new function would have gotten the argument value Main in either case). So this is basically just a bugfix that's being forced by the deprecation of this function. This confusion is basically why moving to @__MODULE__ is a much simpler, more understandable API. (It is a behavioral change if you were calling this while defining a new module, even if called from the Main module, but we didn't have the STDOUT variable available then, so you wouldn't have been able to call this method anyways).

Copy link
Contributor

Choose a reason for hiding this comment

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

you couldn't call whos to a buffer from within a package?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an infinite set of things that someone "could have done" that this PR breaks. I just don't generally care to theorize about them unless someone first shows that it's plausible and meaningful.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's an insulting and unhelpful response to a request to document the breaking behavior changes more clearly.

Copy link
Member Author

Choose a reason for hiding this comment

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

return isconst(_current_module(), s)
end
@noinline function include_string(txt::AbstractString, fname::AbstractString)
depwarn("include_string(string, fname) is deprecated, use `include_string(__module__, string, fname)` instead.", :include_string)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about 1-arg include_string(::AbstractString) ?

doc_str(md::AbstractString, file, mod) = doc_str(parse(md), file, mod)
function doc_str(md, source::LineNumberNode, mod::Module)
md.meta[:path] = source.file
md.meta[:path] = isa(source.file, Symbol) ? String(source.file) : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

is the first assignment to md.meta[:path] redundant?

@@ -842,9 +849,9 @@ end
"""
which(symbol)

Return the module in which the binding for the variable referenced by `symbol` was created.
Return the module in which the binding for the variable referenced by Main.`symbol` was created.
Copy link
Contributor

Choose a reason for hiding this comment

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

also a behavior change for calls to this from non-main modules that should be noted more prominently somewhere

and this should maybe be formatted with Main inside the code highlighting - not sure if Main.$symbol is quite the right way to show it?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I wrote above, current_module returned Main, regardless of where it was called from. Yes, this was highly confusing.

elseif INCLUDE_STATE === 2
result = _include(Base, path)
else
# to help users avoid error (accidentally evaluating into Base), this is deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

if it should become an error eventually, then add a reminder note in deprecated.jl

@@ -530,6 +530,12 @@ LineNumberNode
file: Symbol REPL[2]
```

The argument `__module__` provides information (in the form of a `Module` object)
about the expansion context of the macro invocation.
This allows macros to lookup contextual information, such as existing bindings,
Copy link
Contributor

Choose a reason for hiding this comment

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

"lookup" is a noun, "look up" is a verb

The argument `__module__` provides information (in the form of a `Module` object)
about the expansion context of the macro invocation.
This allows macros to lookup contextual information, such as existing bindings,
or to insert as an extra argument to a runtime function call doing self-reflection.
Copy link
Contributor

Choose a reason for hiding this comment

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

insert what as an extra argument?

@@ -32,7 +32,7 @@ function choosetests(choices = [])
"replutil", "sets", "test", "goto", "llvmcall", "llvmcall2", "grisu",
"nullable", "meta", "stacktraces", "profile", "libgit2", "docs",
"markdown", "base64", "serialize", "misc", "threads",
"enums", "cmdlineargs", "i18n", "workspace", "libdl", "int",
"enums", "cmdlineargs", "i18n", "libdl", "int",
Copy link
Contributor

Choose a reason for hiding this comment

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

put back?

@inline function foo(sub)
it = 1
end
end
end
end
let ex = expand(:(@M16096.iter))
@test !(isa(ex,Expr) && ex.head === :error)
Copy link
Contributor

Choose a reason for hiding this comment

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

why the change in what's being tested here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched from testing that it doesn't fail, to testing that it does succeed (e.g. from a weaker test to a stronger)

@test !isexported(current_module(), :a_value)
@test Base.which_module(@__MODULE__, :a_value) === @__MODULE__
@test @which(a_value) === @__MODULE__
@test_throws ErrorException which(:a_value)
Copy link
Contributor

@tkelman tkelman May 27, 2017

Choose a reason for hiding this comment

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

would this break if tests were run in no-isolation mode? nevermind, this is inside module TestingExported

@vtjnash
Copy link
Member Author

vtjnash commented May 29, 2017

Is the reason this can't be __source__.module that __source__ is about the definition site [parser] whereas __module__ is about the call site [eval]?

Yeah, that's basically why I don't think they should be packed into the same argument

@tkelman
Copy link
Contributor

tkelman commented May 29, 2017

is it really necessary to deprecate 1-argument include_string ? why can't that be made to behave like 1-argument eval ?

@vtjnash
Copy link
Member Author

vtjnash commented May 29, 2017

I want to deprecate that function too (in favor of @eval), I just haven't gotten that PR ready yet. But more importantly, looking though Github search for include_string, the method they usually actually wanted was the one I just added.

@tkelman
Copy link
Contributor

tkelman commented May 29, 2017

then should there be an @include_string ? or even @include_str custom literals?

The argument `__module__` provides information (in the form of a `Module` object)
about the expansion context of the macro invocation.
This allows macros to look up contextual information, such as existing bindings,
or to insert it as an extra argument to a runtime function call doing self-reflection
Copy link
Contributor

Choose a reason for hiding this comment

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

to insert what?

Copy link
Member Author

Choose a reason for hiding this comment

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

the subject of both sentences in the paragraph: the argument __module__

Copy link
Contributor

Choose a reason for hiding this comment

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

does not read clearly

tkelman
tkelman previously requested changes Jun 2, 2017
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

deprecation warning for a call to include_string("foo") should not suggest the user called it with more arguments than they did

@JeffBezanson
Copy link
Member

Is the reason this can't be __source__.module that __source__ is about the definition site [parser] whereas __module__ is about the call site [eval]?

Yeah, that's basically why I don't think they should be packed into the same argument

I'm fine with having them as separate arguments, but I'm a bit confused --- aren't both about the location of the call site?

@vtjnash
Copy link
Member Author

vtjnash commented Jun 2, 2017

I'm fine with having them as separate arguments, but I'm a bit confused --- aren't both about the location of the call site?

Both come from the call site (obviously - that's the definition of being an argument). But one comes syntatically from the parser and one comes dynamically from the scope.

@JeffBezanson
Copy link
Member

Got it.

obviously - that's the definition of being an argument

Obviously, that's not what I meant, which is why I said "about the location of the call site" and not "from the call site".

@vtjnash
Copy link
Member Author

vtjnash commented Jun 2, 2017

Obviously, that's not what I meant

right. that was supposed to be read in jest, but I forget the :)

@tkelman tkelman dismissed their stale review June 5, 2017 16:00

deprecations separated

passes macros a __module__ argument, like the __source__ argument,
allowing them to inspect and configure themselves based on their evaluation context

remove current_module dependence from `include`:
instead, we use the same trick as for `eval` (they are basically the same function after all)
to create a module-relative function in each non-bare module

now puts Module in MethodInstance.def for toplevel thunks to make it convenient to pass around this contextual information
@vtjnash vtjnash merged commit 80369bf into master Jun 6, 2017
@vtjnash vtjnash deleted the jn/isdefined-deprecate branch June 6, 2017 13:51
@JeffBezanson
Copy link
Member

Good change. Nice to be rid of that piece of global state.

The NEWS and deprecation for current_module should more clearly state that @__MODULE__ doesn't do the same thing; it's not a drop-in replacement. Should say that it gives the lexically-enclosing module, and that __module__ should be used for calls from macros.

@vtjnash vtjnash added the needs news A NEWS entry is required for this change label Jun 6, 2017
#define JL_AST_PRESERVE_PUSH(ctx, _roots, old, inmodule) \
jl_array_t *(_roots) = NULL; \
struct { jl_array_t **roots; \
jl_module_t *module; } (old); \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think msvc likes this initializer style

ast.c
c:/projects/julia/src/ast.c(443) : error C2143: syntax error : missing ';' before '<L_TYPE_raw>'
c:/projects/julia/src/ast.c(443) : error C2059: syntax error : '('
c:/projects/julia/src/ast.c(443) : error C2065: 'old_roots' : undeclared identifier
c:/projects/julia/src/ast.c(443) : error C2228: left of '.roots' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(443) : error C2228: left of '.module' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(446) : error C2065: 'old_roots' : undeclared identifier
c:/projects/julia/src/ast.c(446) : error C2228: left of '.roots' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(446) : error C2228: left of '.module' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(967) : error C2143: syntax error : missing ';' before '<L_TYPE_raw>'
c:/projects/julia/src/ast.c(967) : error C2059: syntax error : '('
c:/projects/julia/src/ast.c(967) : error C2065: 'old_roots' : undeclared identifier
c:/projects/julia/src/ast.c(967) : error C2228: left of '.roots' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(967) : error C2228: left of '.module' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(971) : error C2065: 'old_roots' : undeclared identifier
c:/projects/julia/src/ast.c(971) : error C2228: left of '.roots' must have class/struct/union
        type is 'unknown-type'
c:/projects/julia/src/ast.c(971) : error C2228: left of '.module' must have class/struct/union
        type is 'unknown-type'
make[2]: *** [ast.o] Error 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it complaining about the parenthesis? Those aren't necessary, I just used them to make the macro arguments more visible. Or is it complaining that the struct doesn't have a name (https://msdn.microsoft.com/en-us/library/c89bw853(v=vs.100).aspx)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect the later errors are nonsense after the first syntax error. What would it look like if given a name?

@stevengj
Copy link
Member

stevengj commented Nov 2, 2017

I have to say I don't understand why the 1-argument include_string was deprecated here. Vague plans to replace it later don't seem like a good justification.

(That being said, in my ChangePrecision package I found include_string insufficiently flexible and ended up using eval with the undocumented Base.parse_input_line instead.)

StefanKarpinski pushed a commit that referenced this pull request Jan 2, 2018
Ran NEWS-update.jl to refresh link references.
StefanKarpinski added a commit that referenced this pull request Jan 2, 2018
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isdefined() gives unexpected result inside of __init__()
8 participants