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

more predictable global binding resolution #22984

Merged
merged 1 commit into from
Aug 7, 2017
Merged

more predictable global binding resolution #22984

merged 1 commit into from
Aug 7, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jul 27, 2017

This takes any syntactic assignment to a global inside a function and ensure we create the binding immediately.

EDIT: The design of this PR has changed, although the net effect is generally much less breaking (since now the divergence of the new vs. existing behavior was previously typically an error case):

This changes the meaning of `global` from being a directive when used at toplevel,
which forces the introduction of a new global when used in certain contexts,
to always being just an scope annotation that there should exist a global binding for the given name.

fix #18933
fix #17387

@tkelman tkelman added the needs news A NEWS entry is required for this change label Jul 27, 2017
@vtjnash
Copy link
Member Author

vtjnash commented Jul 27, 2017

Of course, CI is failing because a couple of deprecations were written to depend on the old behavior of #17387 and will need an minor update to Compat first to disable this fix in one place in the code.

test/core.jl Outdated
# issue 18933
module GlobalDef18933
using Base.Test
f() = (global sin; nothing)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this is kind of a weird case. Normally global sin at the toplevel would create a binding in the current module. But I see that since f doesn't assign to sin, maybe it shouldn't here. I guess this behavior is probably ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is rather awkward that we use global to mean different operations depending on the scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

even worse, before we weren't even consistent about what this syntax did. for example:

if true
    global sin
    nothing
end

did not create a new global binding, but merely annotated that if a variable named sin was used, that its scope is global.

Copy link
Member

Choose a reason for hiding this comment

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

Ow, good point.

Should add a test for exactly that case.

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 a minor breaking change (but I suspect it will break a handful of packages anyways), but this PR means that:

let
   global function f(); return 1; end
end

Now expands to

global f
f() = 1

And thus will not add to an existing binding from import, but will instead give an overwrite notice.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible we'll require explicit qualification for extension anyway, so this might be moot. However, in the meantime it's possible that global x should not overwrite "hard-imported" bindings. That will be less breaking, since currently it gives a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. That operation gives a warning now, so presumably it would be less breaking for it to be ignored (have global x mean more consistently the statement "x is a global" rather than the being sometimes the command "make x global").

((ssavalue? var)
`(= ,var ,rhs0))
(else
(error (string "invalid assignment location \"" (deparse var) "\"")))))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a good refactoring. Am I right that there's no functional change except for this error?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. I was thinking of using this code-path to test for globals, but ended up deciding to put it elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

maybe put this in a separate PR, especially if it's a completely independent change?

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 largely just a re-indentation to accommodate printing the error message. It seems that lisp just usually makes trivial edits require re-indenting the entire function :(

(if (and (pair? e) (eq? (car e) 'block))
e
`(block ,e))
glob-decl)))))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally convinced it's worth adding this code instead of just looking for global assignments in jl_resolve_globals.

Copy link
Member Author

Choose a reason for hiding this comment

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

This moves the global decls from local scope (where the syntax does not inherently create new bindings), to global scope (where it does). I'm generally inclined to do all scope and syntax lowering here, rather than implementing any part of it in post-processing code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm on board.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, while having jl_resolve_globals do this will mostly work, it's still broken if the global code has any other side-effects, since the current scope rules can't preserve the happens-before relationship the user may have intended. Ignoring the trivial cases like code that uses eval or include, another example of where we must give the wrong behavior is if import is in the same block as global. (so import Foo.bar; global bar() = 3 will execute in the wrong order, for example)

@vtjnash
Copy link
Member Author

vtjnash commented Jul 28, 2017

Make sure that @nanosoldier runbenchmarks(ALL, vs = ":master") isn't unhappy about the syntax chnage.

@vtjnash vtjnash force-pushed the jn/18933 branch 2 times, most recently from 9a0073d to 8a71761 Compare July 28, 2017 18:30
NEWS.md Outdated
* The `global` keyword now always introduces a new binding when appearing in toplevel (global) scope.
Whereas, previously, embedding it in a block (such as `begin; global sin; nothing; end`) would change its meaning.
Additionally, the new bindings are now created before the statement is executed.
For example, `f() = (global sin = rand(); nothing)` will now reliably shadow the `Base.sin` function,
Copy link
Contributor

Choose a reason for hiding this comment

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

no comma at the end of this line

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 comma makes this a dependent clause, which I think is preferable here (the additional statements are present for additional clarity, but aren't as uniformly true as the first statement).

Copy link
Contributor

@tkelman tkelman Jul 28, 2017

Choose a reason for hiding this comment

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

"with a new, undefined sin variable" is an incomplete fragment with no clear subject of its own - it is directly describing the shadowing and doesn't stand on its own

Copy link
Contributor

@tkelman tkelman Aug 3, 2017

Choose a reason for hiding this comment

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

the comma here does not make sense grammatically and should be removed.

Copy link
Member

Choose a reason for hiding this comment

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

this is not important

Copy link
Contributor

Choose a reason for hiding this comment

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

these are the release notes, which people will read, they should read 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.

https://en.wikipedia.org/wiki/Relative_clause#Restrictive_and_non-restrictive

If it was a complete fragment, this would be a comma splice. The rest of the sentence, on the other hand, is complete and stands on its own.

doc/make.jl Outdated
@@ -1,152 +0,0 @@
# Install dependencies needed to build the documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

put this back

Copy link
Member Author

Choose a reason for hiding this comment

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

fortunately, it looks like AppVeyor won't give this PR a passing mark without this (but FreeBSD will). Need to merge a couple PRs to the doc packages first.

@nanosoldier
Copy link
Collaborator

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

vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request Jul 31, 2017
this ensures that we only introduce package-local bindings for values
that we define in this package

ref JuliaLang/julia#22984
vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request Jul 31, 2017
this ensures that we only introduce package-local bindings for values
that we define in this package

ref JuliaLang/julia#22984
test/core.jl Outdated
f() = (global sin; nothing)
g() = (global cos; cos = 2; nothing)
@test @isdefined sin
@test !@isdefined cos
Copy link
Member

@StefanKarpinski StefanKarpinski Aug 1, 2017

Choose a reason for hiding this comment

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

It would be handy here to have some way to test what a binding refers to without dereferencing it, as in resolvebinding(:sin) == :(Base.sin) and resolvebinding(:cos) == :cos, but that may be beyond the scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it has a value, @which. I could add some tests here with it for additional clarity / verification, but I think it's basically already covered.

test/core.jl Outdated
nothing
end
@test !@isdefined sincos
@test isdefined(Base, :sincos)
Copy link
Member

@StefanKarpinski StefanKarpinski Aug 1, 2017

Choose a reason for hiding this comment

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

I find this last one slightly surprising, but I suppose it fall naturally out of if not introducing scope. Should this also test the same things with assignment (without the let)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it's a natural outcome of scope rules (one way to "fix" this might be to remove global scope, ala #19324).

But I'm going to change the way this works in an upcoming commit, which may reduce its impact on typical code.

Copy link
Member Author

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Jeff and I have discussed some of the implications of this fix for the global keyword, and have decided we should change the meaning of the keyword slightly, affecting the error case.

Currently, global means "make a new global" when used as a toplevel statement, but means "there is a global" when used anywhere else. Typically these two statements actually end up doing the same action, and thus differ only in the warning / error case.

This PR had been written to fix the keyword to mean "make a new global" when used in global scope or when there is an assignment to a global in local scope. But to otherwise mean "there is a global".

We have instead decided it should always mean "there is a global". This mainly just impacts cases where we currently give a warning ("WARNING: imported binding for sin overwritten in module Main"), and instead makes that operation a no-op. Because that means bindings can no longer be deleted (which was invalid anyways for various reasons), assignment to an existing global imported from another module will now emit an error ("cannot assign variables in other modules") rather than emitting a warning.

test/core.jl Outdated
f() = (global sin; nothing)
g() = (global cos; cos = 2; nothing)
@test @isdefined sin
@test !@isdefined cos
Copy link
Member Author

Choose a reason for hiding this comment

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

If it has a value, @which. I could add some tests here with it for additional clarity / verification, but I think it's basically already covered.

test/core.jl Outdated
nothing
end
@test !@isdefined sincos
@test isdefined(Base, :sincos)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it's a natural outcome of scope rules (one way to "fix" this might be to remove global scope, ala #19324).

But I'm going to change the way this works in an upcoming commit, which may reduce its impact on typical code.

@vtjnash vtjnash removed the needs news A NEWS entry is required for this change label Aug 3, 2017
src/module.c Outdated
else {
jl_binding_t *b2 = jl_get_binding(b->owner, var);
if (b2 == NULL || b2->value == NULL)
jl_errorf("invalid method definition: imported function %s.%s does not exist", jl_symbol_name(b->owner->name), jl_symbol_name(var));
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap long line

@JeffBezanson
Copy link
Member

I've been kicking the tires on this a bit and it seems good. 👍

This changes the meaning of `global` from being a directive when used at toplevel,
which forces the introduction of a new global when used in certain contexts,
to always being just an scope annotation that there should exist a global binding for the given name.

fix #18933
fix #17387 (for the syntactic case)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants