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

scope of identifier names in lowered syntax forms #25947

Closed
krrutkow opened this issue Feb 8, 2018 · 8 comments · Fixed by #26380
Closed

scope of identifier names in lowered syntax forms #25947

krrutkow opened this issue Feb 8, 2018 · 8 comments · Fixed by #26380
Assignees
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage) parser Language parsing and surface syntax

Comments

@krrutkow
Copy link

krrutkow commented Feb 8, 2018

Is this behavior intended?

julia> f(c) = 1:5 ;

julia> f(0)
1:5

julia> g(colon) = 1:5 ;

julia> g(0)
ERROR: MethodError: objects of type Int64 are not callable
Stacktrace:
 [1] g(::Int64) at ./REPL[3]:1
 [2] top-level scope

If so, is it documented anywhere what variable/function names could change behavior, or could the names used by changed to be more "internal-looking" (e.g. __colon__ instead)?

I can see how the behavior is a potentially useful feature:

julia> function f()
           colon = function (args...) ; println("args: ", join([args...], ' ')) ; Base.colon(args...) ; end
           1:5
       end
f (generic function with 1 method)

julia> f()
args: 1 5
1:5
@JeffBezanson
Copy link
Member

Good catch. Reviewing all such cases (where syntax is transformed to call a function with a name not evident in the code), we're not consistent about this. I think we should change them all to be hygienic; i.e. call Base.colon.

@JeffBezanson JeffBezanson self-assigned this Feb 8, 2018
@JeffBezanson JeffBezanson added the compiler:lowering Syntax lowering (compiler front end, 2nd stage) label Feb 8, 2018
@JeffBezanson
Copy link
Member

Another point specific to this example: we could make : like other operators and use : as its name instead of colon.

@JeffBezanson JeffBezanson added breaking This change will break code parser Language parsing and surface syntax labels Feb 8, 2018
@JeffBezanson JeffBezanson changed the title Affects of variable/function name shadowing scope of identifier names in lowered syntax forms Mar 1, 2018
@JeffBezanson
Copy link
Member

JeffBezanson commented Mar 1, 2018

This is now fixed for colon (#26074)

#26273 has an interesting related example. The macro in question contains basically

quote
    x[i]
    ...
end

x[i] parses to Expr(:ref, :x, :i), so the identifier getindex is not visible to the hygiene pass. This makes it basically impossible for getindex to be lexically scoped. Of course, we could expand a few simple things like ref before the hygiene pass.

There is some good discussion of this in #25957. Our options are basically (1) make these lexically scoped with hygiene support using names like __getindex__, or (2) make them always call Base. The full set of affected functions is:

function name syntax
hcat [a b] *
vcat [a; b] *
hvcat [a b; c d] *
typed_hcat T[a b]
typed_vcat T[a; b]
typed_hvcat T[a b; c d]
vect [a, b]
string "$a $b"
adjoint a'
getindex a[i] *
setindex! a[i] = b *
getproperty a.b
setproperty! a.b = c
broadcast f.(x)
broadcast! a .= b
literal_pow x^2

Items marked (*) are currently lexically scoped (except for the hygiene bug).

Then some borderline cases:

generators: Generator, product, Filter, Flatten, collect

iteration: start, done, next

indexing: lastindex

These are all of the cases where a syntax form corresponds to a function call, but the function name is not evident in the source code.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Mar 1, 2018
@StefanKarpinski
Copy link
Member

I have this funny feeling that Cassette is going to turn out to be the way to do this, but I feel like we may as well do option (1) and allow people to play with this. But I also don't think it's a high priority.

@JeffBezanson
Copy link
Member

Cassette seems a bit excessive when a simple lexically-scoped identifier would do. Also, this is not for transforming other people's code, just writing your own code using a custom definition of getindex.

@JeffBezanson
Copy link
Member

At this point I'm leaning towards making them all call Base for now, and then we can add the double-underscore names and the hygiene fix as a new feature later. It should be non-breaking.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Mar 2, 2018

It should be non-breaking.

It changes the lowered code though, doesn't it? Not saying we can't do that, but we should record it as a caveat and give tips on how to write future-proof code in the Julia API stability doc I've been keeping.

@JeffBezanson
Copy link
Member

Do you have anything in mind that that would break, other than code calling Meta.lower? We should definitely mark that as unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage) parser Language parsing and surface syntax
Projects
None yet
3 participants