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

parse more dot operators, parse .= with assignment precedence #17393

Merged
merged 6 commits into from
Jul 19, 2016

Conversation

stevengj
Copy link
Member

For 0.6, the plan in #16285 is to have a .+ b etcetera automatically correspond to broadcast(+, a, b), and be fused with other "dot calls". Also, we are planning to have x .= ... etcetera turn into broadcast! for in-place fused loops.

As a first step towards that goal, this PR:

  • Changes the precedence of .= to be assignment-like, rather than comparison-like.
  • Makes (nearly) every operator "dottable". The exceptions are operators with very special syntactic meanings, like $ and ||. (Some of these could be allowed, but I wanted to be conservative to start with).

I'd ideally like to get this merged in 0.5. The reason is that, once 0.6 rolls around, we will want a @compat macro to make some of #16285's vectorized syntax work (albeit suboptimally) in 0.5, and it will be hard to do that unless the operators parse.

@stevengj
Copy link
Member Author

(I don't think we should document the new operators, yet, because we don't really want people to start assigning methods to them in 0.5 ... since in 0.6 the broadcast methods will be defined automatically given a method for the non-dotted operator.)

@stevengj
Copy link
Member Author

As far as I can tell, no existing packages use .=, so the change in precedence should be fairly safe. In 0.6, .= will be a (broadcasting) assignment operator so it will not be possible to add methods to it anyway.

@StefanKarpinski
Copy link
Member

👍

@JeffBezanson
Copy link
Member

It seems like we could allow .& and .|.

@stevengj
Copy link
Member Author

stevengj commented Jul 13, 2016

Rebased and added .& and .|

@stevengj
Copy link
Member Author

stevengj commented Jul 13, 2016

@JeffBezanson, I'm bugged by .!. As I understand it, in order to parse .!= as an operator, the parser requires the prefix .! to also be parsed as an operator, which is why .! has to be an operator.

However, the .! operator was parsed with prec-comparison, i.e. as a binary operator with comparison precedence. I kept this behavior, but it seems weird. Shouldn't we parse .! as a unary operator if we parse it at all? Or would that screw up parsing of .!=?

@JeffBezanson
Copy link
Member

We could handle that by rejecting .! at the end of read-operator. That is already done for --.

@stevengj
Copy link
Member Author

stevengj commented Jul 13, 2016

Or maybe we should just allow unary operators to be dotted, and allow .! as a unary op? So that .!x would turn (in 0.6) into broadcast((!), x)?

This seems more consistent, but maybe it requires special handling in the parser to accept a unary .! and a binary .!=?

@stevengj
Copy link
Member Author

(It seems like -- should nowadays be a prec-plus operator, similar to ++.)

@stevengj
Copy link
Member Author

stevengj commented Jul 13, 2016

Okay, I took Jeff's suggestion and reject .! in the parser, like --. Just adding dots before unary operators didn't work (seems to require some deeper changes in the parser for make to succeed), so I punted on that for now. Seems better to err on the side of conservatism when adding new operators, anyway.

With unary operators the need for a dot prefix is somewhat less, because you can always write (!).(array), but it would still be nicer to support .!array eventually.

@stevengj
Copy link
Member Author

Seems like the libgit2 Travis test failure must be unrelated?

@stevengj stevengj closed this Jul 14, 2016
@stevengj stevengj reopened this Jul 14, 2016
@stevengj
Copy link
Member Author

stevengj commented Jul 14, 2016

Restarting checks since the Travis failure on libgit2 should hopefully have been fixed by 74cf1b1, thanks @tkelman

@stevengj
Copy link
Member Author

Rebased and added NEWS.

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

Should this have any parsing tests?

@andyferris
Copy link
Member

I think having .-, .!, etc as unary operators would make a lot of sense, but I understand you said it might be hard to implement.

I can currently do -v and (-).(v), so it seems odd I that can't do .-v.

@stevengj
Copy link
Member Author

@tkelman, added some tests.

@stevengj
Copy link
Member Author

stevengj commented Jul 15, 2016

@andyferris, I don't think it should be that hard to implement more dotted unary operators, it is just that we are so close to the 0.5 release that I don't want to dig deep into the parser right now.

@stevengj
Copy link
Member Author

Okay to merge?

@tkelman tkelman added the parser Language parsing and surface syntax label Jul 16, 2016
@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2016

lgtm but I'm the wrong person to review the scheme code

@stevengj
Copy link
Member Author

@JeffBezanson, looks like it's up to you to hit the "merge" button?

@StefanKarpinski
Copy link
Member

@JeffBezanson is on vacation, so I'd recommend merging if you're happy with it and we can do a post-merge review when he gets back.

@vtjnash
Copy link
Member

vtjnash commented Jul 18, 2016

lgtm. do these deparse (print) correctly?

(also, there's now a trivial merge conflict in test/core, so needs a rebase)

@@ -4462,3 +4462,11 @@ end
f17147(::Tuple) = 1
f17147{N}(::Vararg{Tuple,N}) = 2
@test f17147((), ()) == 2

# PR #17393
Copy link
Member

Choose a reason for hiding this comment

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

put this in test/parse?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@stevengj
Copy link
Member Author

stevengj commented Jul 19, 2016

@vtjnash, yes, the new dotted binary operators print fine. The assignment syntax x .= y prints as an Expr object, but since x .+= y in Julia 0.4 also prints as an Expr object I think that is fine (not a regression anyway)... we should fix printing of dotted assignments once this actually does something.

@stevengj stevengj merged commit 814c974 into JuliaLang:master Jul 19, 2016
@stevengj stevengj deleted the more-dotops branch July 19, 2016 18:28
@mbauman
Copy link
Member

mbauman commented Jul 20, 2016

Oooh, this is cool. Any thoughts here on decoupling the precedence of .| and .& from | and & to support a use case like #5187? That'd be something that'd be good to do in the same cycle that they're introduced since precedences are very difficult to change.

@stevengj
Copy link
Member Author

@mbauman, I find #5187 very hard to follow because of the length of the thread. I would suggest making a PR that changes the precedence, and summarize in the PR what you think are the main arguments in favor from #5187.

@tkelman
Copy link
Contributor

tkelman commented Jul 21, 2016

one note out of this is that we could deprecate elementwise | now that .| is a general version.

@KristofferC
Copy link
Member

KristofferC commented Jul 21, 2016

Did this PR break the following? I haven't bisected but it seems like the .| could be affected by this PR.

julia> Base.|>(a, b) = a+b
ERROR: syntax: ">(a,b)" is not a valid function argument name
 in eval(::Module, ::Any) at ./boot.jl:234
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

(found from Gtk.jl stopped parsing):

@stevengj
Copy link
Member Author

stevengj commented Jul 21, 2016

The dot operators are now parsed, but the don't do anything different from 0.4 in this PR.

Deprecations will have to wait until 0.6 when dot operators are sugar for a broadcast.

@KristofferC
Copy link
Member

Ok, I guess something else made that syntax fail to parse because it worked in 0.4 and it is currently failing. That fact that the error message says that >(a,b) is not a valid function name (the .| part being removed from the error message) made me suspect that this PR had something to do with it,

@stevengj
Copy link
Member Author

stevengj commented Jul 21, 2016

@KristofferC, sorry, I was replying to @tkelman. Probably this PR broke Base.|>. You should now write Base.:|>, the same way that you have to write Base.:+ and not Base.+

@tkelman
Copy link
Contributor

tkelman commented Jul 21, 2016

Also broke IntervalConstraintProgramming and LibExpat

dpsanders pushed a commit to JuliaIntervals/IntervalConstraintProgramming.jl that referenced this pull request Jul 24, 2016
@stevengj stevengj added the broadcast Applying a function over a collection label Aug 2, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
…ang#17393)

* parse more dot operators for JuliaLang#16285, parse .= with assignment precedence

* include .|= and .&= in syntactic-operators list

* reject .! operator

* NEWS for new operators

* tests

* move tests to parse.jl
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants