replace current_module() with @MODULE #22064

vtjnash · 2017-05-25T22:39:23Z

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

tkelman · 2017-05-26T03:18:31Z

base/reflection.jl

+"""
+    @isdefined s -> Bool
+
+Get whether symbol `s` is defined in the current scope.


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

tkelman · 2017-05-26T03:19:50Z

src/builtins.c

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


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

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

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

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

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 · 2017-05-26T16:15:22Z

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

vtjnash · 2017-05-26T17:18:48Z

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 · 2017-05-26T17:35:27Z

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 · 2017-05-26T17:58:20Z

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

StefanKarpinski · 2017-05-26T18:38:26Z

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

tkelman · 2017-05-27T07:09:09Z

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

nanosoldier · 2017-05-27T13:01:05Z

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

tkelman · 2017-05-27T16:52:42Z

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

tkelman · 2017-05-27T16:57:15Z

base/deprecated.jl

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


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

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).

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

tkelman · 2017-05-27T17:02:26Z

base/interactiveutil.jl


 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"")


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

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).

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

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.

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

also added documentation of change: https://github.com/JuliaLang/julia/pull/22064/files#diff-8312ad0561ef661716b48d09478362f3R68

tkelman · 2017-05-27T17:04:40Z

base/deprecated.jl

+    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)


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

tkelman · 2017-05-27T17:12:23Z

base/markdown/Markdown.jl

-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) : ""


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

tkelman · 2017-05-27T17:20:46Z

base/reflection.jl

@@ -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.


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?

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

tkelman · 2017-05-27T17:23:56Z

base/sysimg.jl

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


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

tkelman · 2017-05-27T17:28:56Z

doc/src/manual/metaprogramming.md

@@ -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,


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

tkelman · 2017-05-27T17:29:49Z

doc/src/manual/metaprogramming.md

+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.


insert what as an extra argument?

tkelman · 2017-05-27T17:51:41Z

test/choosetests.jl

@@ -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",


tkelman · 2017-05-27T17:55:38Z

test/parse.jl

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


why the change in what's being tested here?

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

tkelman · 2017-05-27T17:58:18Z

test/reflection.jl

-@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)


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

vtjnash · 2017-05-29T01:17:43Z

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 · 2017-05-29T02:39:52Z

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

vtjnash · 2017-05-29T03:04:08Z

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 · 2017-05-29T03:11:43Z

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

tkelman · 2017-06-02T13:56:57Z

doc/src/manual/metaprogramming.md

+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


to insert what?

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

does not read clearly

tkelman

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

JeffBezanson · 2017-06-02T14:23:04Z

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 · 2017-06-02T14:45:17Z

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 · 2017-06-02T14:47:15Z

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 · 2017-06-02T15:31:52Z

Obviously, that's not what I meant

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

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

JeffBezanson · 2017-06-06T18:13:38Z

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.

tkelman · 2017-06-07T22:42:50Z

src/ast.c

+#define JL_AST_PRESERVE_PUSH(ctx, _roots, old, inmodule)  \
+    jl_array_t *(_roots) = NULL;                \
+    struct { jl_array_t **roots;                \
+             jl_module_t *module; } (old);      \


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

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)?

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

stevengj · 2017-11-02T16:38:27Z

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.)

Ran NEWS-update.jl to refresh link references.

Fix NEWS.md links for grouped #22064 and #24278

vtjnash force-pushed the jn/isdefined-deprecate branch from 434b133 to 6a9153c Compare May 25, 2017 23:11

tkelman reviewed May 26, 2017

View reviewed changes

vtjnash force-pushed the jn/isdefined-deprecate branch from 6a9153c to 96d6b91 Compare May 26, 2017 14:04

kshyatt added the modules label May 27, 2017

tkelman reviewed May 27, 2017

View reviewed changes

vtjnash mentioned this pull request May 29, 2017

Wrong paths for docstrings #22105

Closed

vtjnash force-pushed the jn/isdefined-deprecate branch from 96d6b91 to 898c5c5 Compare May 29, 2017 02:06

vtjnash force-pushed the jn/isdefined-deprecate branch from d16a141 to 3452d34 Compare June 2, 2017 13:37

tkelman reviewed Jun 2, 2017

View reviewed changes

tkelman previously requested changes Jun 2, 2017

View reviewed changes

vtjnash force-pushed the jn/isdefined-deprecate branch from 3452d34 to 1793f20 Compare June 5, 2017 15:58

vtjnash force-pushed the jn/isdefined-deprecate branch from 1793f20 to 3c3ced4 Compare June 6, 2017 02:49

vtjnash merged commit 80369bf into master Jun 6, 2017

vtjnash deleted the jn/isdefined-deprecate branch June 6, 2017 13:51

vtjnash added the needs news A NEWS entry is required for this change label Jun 6, 2017

timholy mentioned this pull request Jun 7, 2017

Support atsign-__MODULE__ JuliaLang/Compat.jl#363

Merged

tkelman reviewed Jun 7, 2017

View reviewed changes

stevengj mentioned this pull request Jun 21, 2017

Add support for Core.include JuliaLang/Compat.jl#370

Closed

vtjnash mentioned this pull request Jun 28, 2017

Calling workspace on remote workers doesn't work correctly #17433

Closed

fredrikekre mentioned this pull request Oct 23, 2017

deprecate expand(module, ex) to Meta.lower(module, ex) #24278

Merged

stevengj mentioned this pull request Nov 2, 2017

include_string causes spurious AbstractRange deprecation warnings #24447

Closed

StefanKarpinski pushed a commit that referenced this pull request Jan 2, 2018

Fix NEWS.md links for grouped #22064 and #24278

973e548

Ran NEWS-update.jl to refresh link references.

StefanKarpinski added a commit that referenced this pull request Jan 2, 2018

Merge pull request #25350 from jayschwa/fix-news-links

6729e4f

Fix NEWS.md links for grouped #22064 and #24278

ihnorton mentioned this pull request Feb 5, 2018

add builtin @__FUNCTION__ macros #6733

Open

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

stevengj mentioned this pull request Jun 25, 2018

updates for 0.7 JuliaInterop/NBInclude.jl#10

Merged

jekbradbury mentioned this pull request Aug 9, 2018

Implement @isdefined JuliaLang/Compat.jl#609

Closed

stevengj mentioned this pull request Aug 28, 2018

Added @softscope macro for ease of use JuliaLang/SoftGlobalScope.jl#1

Merged

KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018

replace current_module() with @__MODULE__ #22064

replace current_module() with @__MODULE__ #22064

Conversation

vtjnash commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented May 26, 2017

vtjnash commented May 26, 2017

StefanKarpinski commented May 26, 2017

vtjnash commented May 26, 2017 • edited Loading

StefanKarpinski commented May 26, 2017

tkelman commented May 27, 2017

nanosoldier commented May 27, 2017

tkelman commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman May 27, 2017 • edited Loading

Choose a reason for hiding this comment

vtjnash commented May 29, 2017

tkelman commented May 29, 2017

vtjnash commented May 29, 2017

tkelman commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman left a comment

Choose a reason for hiding this comment

JeffBezanson commented Jun 2, 2017

vtjnash commented Jun 2, 2017

JeffBezanson commented Jun 2, 2017

vtjnash commented Jun 2, 2017

JeffBezanson commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Nov 2, 2017 • edited Loading

replace current_module() with @MODULE #22064

replace current_module() with @MODULE #22064

vtjnash commented May 26, 2017 •

edited

Loading

tkelman May 27, 2017 •

edited

Loading

stevengj commented Nov 2, 2017 •

edited

Loading