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

RFC: Deprecate several space insensitivities #11891

Merged
merged 2 commits into from Jul 1, 2015
Merged

RFC: Deprecate several space insensitivities #11891

merged 2 commits into from Jul 1, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jun 27, 2015

The overall agreement in #7232 seems to be that we should deprecate f (17) and now is as good of a time as any to go ahead and do this. This PR takes this even further and deprecates

  1. f (17)
  2. x [17]
  3. rand(17) [17]
  4. function f () 17 end

I also considered deprecating f .env and f. env, but I can see the use for them when it comes to chaining calls. However, an argument for deprecating them would be that we are trying to push the idea that Julia is not a dot-oriented language. Deprecating the dots as well could then allow us to push this direction further. Any thoughts on the matter?

I had never written a single line of Lisp until I started on this PR, feedback is most welcome.

@tkelman tkelman added the parser Language parsing and surface syntax label Jun 27, 2015
@ghost ghost changed the title WIP: Deprecate f (...), x [...] and f {T} syntax RFC: Deprecate f (...), x [...] and f {T} syntax Jun 27, 2015
@yuyichao
Copy link
Contributor

I'm glad that you make it.

Should mention that 1-3 are parsed differently when using inside [].

@ghost
Copy link
Author

ghost commented Jun 27, 2015

Should mention that 1-3 are parsed differently when using inside [].

Sadly, yes, but is there really anything that we can do about [sin (17)]?

@ScottPJones
Copy link
Contributor

@yuyichao Can you explain the differences, and the rationale behind the differences in parsing? Thanks!

@yuyichao
Copy link
Contributor

Sadly, yes, but is there really anything that we can do about [sin (17)]?

Not really anything now but this is the most appealing and objective reason for me to deprecate (and possibly disallow) sin (17).

@ghost
Copy link
Author

ghost commented Jun 27, 2015

@ScottPJones: The issue stems from the following syntax for columns and rows.

julia> [17 4711]
1x2 Array{Int64,2}:
 17  4711

julia> [17, 4711]
2-element Array{Int64,1}:
   17
 4711

@ScottPJones
Copy link
Contributor

Is that going to be changed during Arraymageddon? Essentially having a different language
just because you are between [ ] drives me crazy (and I imagine other people as well)

@ghost
Copy link
Author

ghost commented Jun 27, 2015

@ScottPJones: I don't think there are any plans to change it, but feel free to open an issue to discuss it. As it stands, while related, it is really beyond the scope of this PR.

@ghost
Copy link
Author

ghost commented Jun 27, 2015

I should add, if I get someone to agree to merge this, I will go ahead and add a commit to the PR that fixes occurrences of the deprecated syntax in base.

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

ref #7128 for that one

@mbauman
Copy link
Sponsor Member

mbauman commented Jun 27, 2015

+100.

I also considered deprecating f .env and f. env, but I can see the use for them when it comes to chaining calls

Of all the cases you found and deprecated, this one surprised me the most. I don't understand what you mean by chaining here — could you give an example where this is useful?

@ghost
Copy link
Author

ghost commented Jun 27, 2015

Of all the cases you found and deprecated, this one surprised me the most. I don't understand what you mean by chaining here — could you give an example where this is useful?

Something like:

g(f(17).foo.bar).qux

However, I think this is really carrying over idioms from "properly" object oriented languages. I would opt for:

bar(foo) = foo.bar # Defined previously somewhere.
qux(g(bar(f(17))))

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

g(f(17).foo.bar).qux

You're not deprecating that here though, right? Just saying that maybe putting spaces into that would look nicer to some people? I say down with spaces there too.

@ghost
Copy link
Author

ghost commented Jun 27, 2015

@tkelman: Nope, not touching dots yet, I am tempted though.

@mbauman
Copy link
Sponsor Member

mbauman commented Jun 27, 2015

(on master):

julia> log(3+5im) . re
1.7631802623080808

julia> [log(3+5im)  .  re]
1-element Array{Float64,1}:
 1.76318

julia> Nullable(log)  .  value  (3)
1.0986122886681096

julia> [Nullable(log)  .  value  (3)]
1x2 Array{Any,2}:
 log  3

Wild.

@ghost
Copy link
Author

ghost commented Jun 27, 2015

@mbauman: Holy smokes, that is Pandora's box right there...

@ScottPJones
Copy link
Contributor

I'd vote 👍 for not allowing spaces between the . also.
I thought that #7128 meant that [a b] would not be allowed, but it's hard to tell reading #7128 just what the final decision was (or was there a decision?) to what replaces it?

@ghost ghost changed the title RFC: Deprecate f (...), x [...] and f {T} syntax RFC: Deprecate f (...), x [...], f {T}, and x .y syntax Jun 27, 2015
@ghost
Copy link
Author

ghost commented Jun 27, 2015

I took the plunge and deprecated the x .y syntax as well. x. y is proving to be trickier since I think I need to hack it in at the time of tokenisation.

@ghost
Copy link
Author

ghost commented Jun 27, 2015

x. y deprecation is in, I am not all that pleased with the warning though:

julia> sin. fptr

WARNING: deprecated syntax ". ".
Use "." instead.
Ptr{Void} @0x00007f6e716578c0

Digging through the Femtolisp implementation I was unable to find a way to peek more than one character ahead. If it is possible to shift this deprecation until after tokenisation is done.

As a bonus, import Base. BLAS is now deprecated. But f {T}(x::T) = T and using Base .BLAS are still allowed. They are up next. Shifting the issue back to WIP, but bring on the comments.

@ghost ghost changed the title RFC: Deprecate f (...), x [...], f {T}, and x .y syntax WIP: Deprecate f (...), x [...], f {T}, and x .y syntax Jun 27, 2015
@ghost
Copy link
Author

ghost commented Jun 27, 2015

Correction, f {T}(x::T) = T is already deprecated.

@ghost ghost changed the title WIP: Deprecate f (...), x [...], f {T}, and x .y syntax RFC: Deprecate f (...), x [...], f {T}, and x .y syntax Jun 28, 2015
@JeffBezanson
Copy link
Sponsor Member

👍

@johnmyleswhite
Copy link
Member

I'm not sure we should drop the x .y stuff since I can imagine people using OO-style method chaining if getfield becomes overloadable.

@StefanKarpinski
Copy link
Sponsor Member

We can always re-allow it if and when we get there.

@ghost
Copy link
Author

ghost commented Jun 28, 2015

I have been able to deprecate everything on the form $FOO. $BAR, but $FOO .$BAR got too darn hairy and I simply ripped it out in the end. There was too much code copying and I don't think rewriting portions of our parser with less than 24 hours of exposure to parsing and Lisp is a particularly good idea. I am fine with merging this once the CI goes green.

@ghost ghost changed the title RFC: Deprecate f (...), x [...], f {T}, and x .y syntax RFC: Deprecate several space insensitivities Jun 28, 2015
@johnmyleswhite
Copy link
Member

We can always re-allow it if and when we get there.

Works for me.

Pontus Stenetorp added 2 commits June 29, 2015 15:03
Deprecates:

* `f (...)` #7232
* `x [...]`
* `f {T}`
* `x .y`
* `import x .y`
* `use x .y`
@ghost
Copy link
Author

ghost commented Jun 29, 2015

Pushed the removals of the deprecated syntax from base. I would like to emphasise that I was trying to remain consistent with the surrounding code rather than trying to establish some sort of new consistent standard when it comes to aligning lines of code.

@ghost
Copy link
Author

ghost commented Jun 29, 2015

Also, for future reference. Running ../julia -e 'include("sysimg.jl")' 2> "${FOO}" in base was the best way I found to catch deprecated syntax in base.

@ghost
Copy link
Author

ghost commented Jul 1, 2015

The CI failure appears to be the same good old unrelated time out.

jakebolewski added a commit that referenced this pull request Jul 1, 2015
RFC: Deprecate several space insensitivities
@jakebolewski jakebolewski merged commit 3aed3f5 into JuliaLang:master Jul 1, 2015
jakebolewski added a commit that referenced this pull request Jul 1, 2015
@johnmyleswhite
Copy link
Member

Victory!!!!!

@tkelman tkelman mentioned this pull request Jul 1, 2015
@ghost
Copy link
Author

ghost commented Jul 1, 2015

@jakebolewski: Thank you for the news entry and sorry about forgetting to include it myself.

@timholy
Copy link
Sponsor Member

timholy commented Jul 1, 2015

I like this very much, but worth pointing out that it seems a likely culprit in breaking Tk (and any package that depends on Tk). JuliaGraphics/Tk.jl#104

@tkelman
Copy link
Contributor

tkelman commented Jul 1, 2015

@timholy that was much more likely f7a4aa3, this PR was intended to be non-breaking

@tkelman
Copy link
Contributor

tkelman commented Jul 1, 2015

ref JuliaGraphics/Tk.jl@f283145 where @carnaval had a temp fix but not sure if it's suitable for broader consumption

@jdlangs
Copy link
Contributor

jdlangs commented Jul 8, 2015

Was this intended to become an error and not a deprecation?

julia> x = rand(5);

julia> x [1];
ERROR: syntax: malformed expression

@JeffBezanson
Copy link
Sponsor Member

That usually indicates a nasty internal error, basically an uncaught exception in the parser.

@IainNZ
Copy link
Member

IainNZ commented Jul 9, 2015

Oh I thought that was intential, it happened in a few packages (only can detect in ones that were passing/loading already) (http://pkg.julialang.org/pulse.html) - a third of those are from JSON, another third from this, and the rest misc I guess. They all seem to be of the general pattern of @phobon's example though.

@jakebolewski
Copy link
Member

should be fixed now

@stev47
Copy link
Contributor

stev47 commented Aug 22, 2015

Regarding the spaces: deprecating f (x) is fine for calls but I'm not happy about deprecating definitions like

function add (x, y)
    x + y
end

The space after add separates the arguments visually. This is even included in several coding conventions of other languages. If there are no other issues with this I'd really like to keep that.

@nalimilan
Copy link
Member

Why would you want to use a different convention for defining vs. calling a function? Sounds like a source of inconsistencies without a clear gain to me.

@StefanKarpinski
Copy link
Sponsor Member

Not to mention that the code that parses them is the same – this would necessitate making them different.

@stev47
Copy link
Contributor

stev47 commented Aug 22, 2015

Well, defining a function and calling a function are clearly different things, so I don't think it's an inconsistency. It's just that I was forced to change that spacing I've always done up until now to get it working and I don't quite see why. So in my eyes you are breaking code for people without a good reason (apart from implementation convenience).
It's not that julia enforces a hard style guide regarding spacing, so this shouldn't be an issue.

@StefanKarpinski
Copy link
Sponsor Member

Sorry for the hassle. This shell command should fix it:

find . -name '*.jl' | xargs perl -i -ple 's/^(\s*function\s+[\w!]+)\s+\(/$1(/'

There's no choice that makes allowing space in function definition but not invocation consistent:

f (x)            # not allowed
f (x) = 2x + 1   # ???
function f (x)   # allowed
    2x + 1
end

No matter what you choose for the middle, it's inconsistent. The only really consistent way to go is to allow all three or none.

The fact that it would cause annoying code forking in the parser is simply a symptom of the fact that it's introducing two different – and thus by definition inconsistent – ways of parsing similar syntaxes. If you don't believe me, I invite you to have a go at changing the parser to allow the space in function definitions and but not in function invocations.

@stev47
Copy link
Contributor

stev47 commented Aug 22, 2015

Thank you for the explanation, I now see your point with f(x) = 2x + 1. It's probably alright then, I'll be changing my habits.
Haven't had the pleasure to dive into a parser yet, but I do believe you nonetheless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.