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

deprecate using _ as an rvalue #20328

Merged
merged 1 commit into from
Feb 8, 2017
Merged

deprecate using _ as an rvalue #20328

merged 1 commit into from
Feb 8, 2017

Conversation

JeffBezanson
Copy link
Member

This is a minimal change to get a warning for accessing the value of _. We need to decide whether this extends to eval(:_) and Mod._. I'm also not sure we want/need to allow multiple _ as argument names in this release; that will take more work.

@ararslan ararslan added deprecation This change introduces or involves a deprecation parser Language parsing and surface syntax labels Jan 30, 2017
@JeffBezanson JeffBezanson added this to the 0.6.0 milestone Jan 30, 2017
@JeffBezanson JeffBezanson removed the parser Language parsing and surface syntax label Jan 30, 2017
@StefanKarpinski
Copy link
Member

For now this seems sufficient to me. I'm curious what the thumbs downs are about.

@tkelman
Copy link
Contributor

tkelman commented Jan 30, 2017

I don't really like having this special cased as a variable name you can't use. Will this warning hit macro authors that try to use _ in a dsl? ok probably not

@tkelman
Copy link
Contributor

tkelman commented Jan 31, 2017

If most of the goal here is for unused lvalues, I have an idea that's more verbose (and a bit of a pun) but doesn't require any modified syntax:

julia> import Base: setindex!, DevNullStream
julia> setindex!(::DevNullStream, x) = nothing
setindex! (generic function with 94 methods)
julia> a, DevNull[], DevNull[] = (1, 2, 3)
(1,2,3)
julia> a
1

@bramtayl
Copy link
Contributor

bramtayl commented Jan 31, 2017

Several chaining packages currently use _ extensively.

Edit: we've got magic to do, just for you. Join us!

@amitmurthy
Copy link
Contributor

There is a precedent for this in Erlang - http://stackoverflow.com/questions/13707361/anonymous-variables-in-erlang

@ararslan
Copy link
Member

@amitmurthy I'd actually argue that your link shows that "magical" things like this introduce potentially confusing special cases.

@amitmurthy
Copy link
Contributor

It ceases to be magical once documented and widely used. And then it gets known as a feature of the language. I found it quite useful when writing code in Erlang.

@JeffBezanson
Copy link
Member Author

I don't believe this will affect uses in macros. If the macro renames _ to something else it should still work with no warning.

One problem with the DevNull[] approach is it can't be used for function argument names.

@tkelman
Copy link
Contributor

tkelman commented Jan 31, 2017

There's less need for syntax magic there though, right? ::Any or _1, _2, _3 etc already work.

In most languages that have this as special syntax, isn't it playing a role in pattern matching of some kind?

@JeffBezanson
Copy link
Member Author

I believe the use in pattern matching is basically compatible; it matches anything and discards it.

@bramtayl
Copy link
Contributor

bramtayl commented Feb 1, 2017

If the macro renames _ to something else it should still work with no warning.

Do you mean:

  1. Preprocessing code to replace all _ with some gensym before running it through chaining? This would be error prone because there would be 2 different meanings of _

  2. Getting users to use a different symbol
    I'd be ok with this but what would be a good alternative?

@JeffBezanson
Copy link
Member Author

What I mean is that writing @mac f(_) in a program would not itself be an error. There would only be an error if that macro expands to code that reads a variable called _.

@StefanKarpinski
Copy link
Member

In fact, this effectively clears the use of _ for DSLs as being entirely safe, whereas now if they treat _ specially now, they are potentially blocking the usage of an actual value bound to _.

@tkelman
Copy link
Contributor

tkelman commented Feb 2, 2017

Looking at existing uses of this in packages, there are fair number of uses of the form _ -> f(_), or [f(_) for _ in a] that this deprecation would hit.

@StefanKarpinski
Copy link
Member

Can you quantify "fair number"?

@tkelman
Copy link
Contributor

tkelman commented Feb 2, 2017

These are some messy greps, kinda need to manually look through with some meaningful and some (most) not. I'm even seeing _ used as a field name in someone's type, which surprises me a little.

also a few places where it's used for a type parameter

@JeffBezanson
Copy link
Member Author

It's weird, but using it as a field name could still be allowed maybe.

@JeffBezanson
Copy link
Member Author

I'm still thinking we should do this. The idea is not a global ban on the symbol _, so it's still allowed as a field name and anywhere else you can manage to use it. If we end up not liking it, we can switch back.

@tkelman
Copy link
Contributor

tkelman commented Feb 6, 2017

I already don't like it, it's an odd special case that this is a valid identifier name that you aren't allowed to use. And that we're disallowing rvalue usage in order to later have an lvalue-ignore feature.

@JeffBezanson
Copy link
Member Author

It's a relatively innocuous kind of special case, since it gives an early error. As opposed to, say, _ being usable but having some different behavior at run time.

@StefanKarpinski
Copy link
Member

Merging this affords us the option to use _ as a special name in pattern matching, as a name that discards assignments, as a placeholder in pipelines, etc. There are not a whole lot of options for "special names" – this is one of the only ones we could possibly use. If we don't do any of those things, it can go back to just being the worst variable name ever.

@stevengj
Copy link
Member

stevengj commented Feb 7, 2017

Would be great to have this as a first step towards #9343.

@JeffBezanson JeffBezanson merged commit 8fd280f into master Feb 8, 2017
@JeffBezanson JeffBezanson deleted the jb/deprecate_ branch February 8, 2017 14:53
@StefanKarpinski
Copy link
Member

The simplest case doesn't actually get caught by this deprecation:

julia> _ = 1
1

julia> _
1

julia> _ + 0

WARNING: deprecated syntax "_ as an rvalue".
1

Probably a special case where we skip eval when the input is just a symbol.

@stevengj
Copy link
Member

stevengj commented Feb 8, 2017

Good catch @StefanKarpinski. It's doing eval, and in fact eval(:_) has the same issue. The problem is due to these two lines, I think.

@stevengj
Copy link
Member

stevengj commented Feb 8, 2017

Probably jl_eval_global_var should be fixed to add a deprecation message as well if the variable is :_.

@StefanKarpinski
Copy link
Member

Good hunting – I did not manage to track that down.

@timholy
Copy link
Member

timholy commented Feb 18, 2017

I think this is giving a spurious warning in ImageFiltering.

shell> touch ImageFiltering.jl

julia> using ImageFiltering
INFO: Recompiling stale cache file /home/tim/.julia/lib/v0.6/ImageFiltering.ji for module ImageFiltering.

WARNING: deprecated syntax "_ as an rvalue".

WARNING: deprecated syntax "_ as an rvalue".

I've checked the results of grep "=.*_" * (to ensure _ is to the right of =) three times without seeing anything.

Requires JuliaImages/ImageFiltering.jl#25 if you don't want a sea of other warnings. And the build is broken current due to #20658, but you can work around that by commenting out the second reshape specialization in FFTViews.

@stevengj
Copy link
Member

Could it be in some macro defined by another package that ImageFiltering is using?

@timholy
Copy link
Member

timholy commented Feb 21, 2017

Could be. Since this is a parser depwarn, is it possible to produce a more useful message localizing the problem? One already has to do a certain amount of work even to localize which package(s) are producing the warning. Usually I solve such problems by grepping my whole package directory for the offending string, but in this case that's not really practical:

tim@diva:~/.julia/v0.5$ fls=$(find . -name "*.jl")
tim@diva:~/.julia/v0.5$ grep "=.*_" $fls | wc
  35049  215863 3267100

For small packages it's usually pretty easy to find, but for big ones I think the only option is to copy-paste each file at the REPL and scan the output. That's pretty painful.

I had the impression that parser depwarns are "easy," so I'm hopeful that the lack of useful file/line info is just an oversight. But since it's only for rvalues maybe this is a more complicated case? In any event, if it is easy, it would be an enormous help to people who are trying to get their packages ready for 0.6 to fix this sooner rather than later. CC @JeffBezanson.

@carlobaldassi
Copy link
Member

@timholy in the meantime, you could refine that regex like this grep "=.*\<_\>" $fls | wc
(\< and \> are word delimiters, it will miss things like 3_ which used to be valid juxtaposition multiplications, but I hope nobody's doing that. It will also miss cases in which the rhs expression is broken in multiple lines, but this is also true for the previous regex, or any regex really, you need the full parser for that.)

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
@tkelman tkelman removed the needs news A NEWS entry is required for this change label May 14, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
@oxinabox oxinabox mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants