disallow 'global x' when x exists in outer scope

[ihnorton]

"fixes" #7264
before:

julia> let
         z = 4
              let
                  global z = 10
              end
         end
10
julia> z
ERROR: UndefVarError: z not defined

after:

let
       z = 4
       let
           global z = 10
       end
end
ERROR: syntax: variable "z" declared local in enclosing scope

I guess ideally this ought to work, but at least giving an error makes the problem obvious immediately.

Alternatively, if someone can suggest how to pass the annotation through I'll try to implement that.(...remember the variable and emit a a lambda when compiling the assignment block? edit: ick, nevermind. that would be order-dependent)

[tkelman]

[av skip]

This is changing code, so not a very good candidate for that? Stefan upgraded to a second worker so the queue is completely caught up now.

[JeffBezanson]

This change seems like a good idea.

[ScottPJones]

LGTM 👍

[ihnorton]

@tkelman I want to tweak the error message a little bit, and will let av run before merging. Will add NEWS entry too, I guess.

(it would be bizarre for this to affect Windows only, but I suppose stranger things have happened. Good to know that we have more queue space now)

[mauro3]

Regarding @ihnorton's comment "ideally this ought to work". I think maybe it shouldn't as it would be confusing. Is there any situation where z in the middle layer could not just be renamed to resolve the conflict?

Also, maybe the error message could read: `variable "z" is a local variable in the enclosing scope." as it was not declared local.

[ihnorton]

Also, maybe the error message could read: `variable "z" is a local
variable in the enclosing scope." as it was not declared local.

will do.

I can't think of a counter-example that would necessitate allowing this,
and I agree that doing so would permit bad coding habits/style...
disallowing it just feels subtly inconsistent.

On Thu, Jul 2, 2015 at 1:26 PM, Mauro notifications@github.com wrote:

Regarding @ihnorton https://github.com/ihnorton's comment "ideally this
ought to work". I think maybe it shouldn't as it would be confusing. Is
there any situation where z in the middle layer could not just be renamed
to resolve the conflict?

Also, maybe the error message could read: `variable "z" is a local
variable in the enclosing scope." as it was not declared local.

—
Reply to this email directly or view it on GitHub
#11985 (comment).

[mauro3]

You're right about the inconsistency, but probably this is good enough.

[ScottPJones]

I'm a big fan of consistency, but sometimes other considerations are more important, which I think is the case here.

[mauro3]

This is a bit odd:

julia> function f(x)
       global x
       x 
       end
ERROR: syntax: `global x`: x is local variable in the enclosing scope

but probably nothing to worry about.

[pao]

Why is it odd? f intentionally collides the names. Are you just commenting on the error message, which could be slightly more specific and mention that it's a method parameter in this case? Or is there something else I'm not seeing here?

[mauro3]

Sorry, I should have been more clear: it's just the error message that could be better in this case. As far as the user is concerned, there is no "enclosing scope" which could have locals as it is the global scope.

(I suspect that under the hood the function arguments are in a local scope and the body is then a nested local scope of that.)

[coveralls]

Changes Unknown when pulling 9a4c0d8 on ihn/re7264 into ** on master**.

[martinholters]

Why are we getting these every once in a while?

[tkelman]

Coveralls hash collision is my guess.