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: remove default importall Base.Operators #12235

Merged
merged 4 commits into from
Jul 29, 2015

Conversation

JeffBezanson
Copy link
Member

Here is what this might look like. It's not so bad, and I was pleasantly surprised to see that several submodules of Base already explicitly imported the operators they extend, which is a good sign. However life is very very hard without a default import of Base.call, so I think we're stuck with that for now. Still an improvement over importing the 73 identifiers in Base.Operators though.

ref #8113

@tkelman
Copy link
Contributor

tkelman commented Jul 20, 2015

At least it's a simple fix, but this is going to need to be updated in soo many packages. Definitely needs mentioning in NEWS.md, and a massive package PR spree for cleanup.

@quinnj
Copy link
Member

quinnj commented Jul 21, 2015

PkgEvaluator @IainNZ?

@samuelpowell
Copy link
Member

+1: Current situation is ostensibly arbitrary, smells magical, and generates conflicts. Package fix is trivial.

@tkelman
Copy link
Contributor

tkelman commented Jul 21, 2015

Anyone who +1's this better help with making all of the necessary PR's.

I could make test binaries off this branch pretty easily if the buildbots weren't screwed up and out of disk space right now.

@tkelman
Copy link
Contributor

tkelman commented Jul 21, 2015

Also I'll say this so Iain doesn't have to. PackageEvaluator doesn't have to be run by him (it hasn't, in 11 days, since he's been at a conference and otherwise occupied). For quite some time it's been possible to run anywhere, by anyone who has Vagrant, disk space and cpu cycles.

@IainNZ
Copy link
Member

IainNZ commented Jul 21, 2015

Indeed. Also, the machine it was running on had to be moved to my house, but I've had to turn it off because it was a giant space heater. Not sure how long until I can get it back up at another location.

@IainNZ
Copy link
Member

IainNZ commented Jul 21, 2015

Oh wow I just realized what this PR is. Is there no way we can hack in some deprecation warning for operators being overload without this for this release and do it right after we start on 0.5?

@JeffBezanson JeffBezanson force-pushed the jb/dontimportoperators branch from 2f6a8e4 to 1dde6e1 Compare July 28, 2015 21:31
@JeffBezanson
Copy link
Member Author

Ok, I have pulled out all the stops to make this non-breaking, but give warnings if you rely on the old behavior. It even found an import I missed in the first commit.

@IainNZ
Copy link
Member

IainNZ commented Jul 28, 2015

Sounds great Jeff, I think its worth the effort!

@tkelman
Copy link
Contributor

tkelman commented Jul 29, 2015

Much better. Still maybe worth mentioning in NEWS as a deprecation?

@mdcfrancis
Copy link

https://groups.google.com/forum/#!topic/julia-users/o3mfc0vfsz8

I'm stunned by this change - so if I read it correctly in 0.5 Julia will allow accidental shadowing of basic operators?

@IainNZ
Copy link
Member

IainNZ commented Sep 1, 2015

It'll allow it, but you'll get a warning (if I understand correctly) (plus possible hilarity if its a really important method):

              _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-pre+7092 (2015-08-29 02:04 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 2af4698 (3 days old master)
|__/                   |  x86_64-apple-darwin13.4.0

julia> +(x::Int,y::Int) = x + y
WARNING: module Main should explicitly import + from Base
WARNING: Method definition +(Int64, Int64) in module Base at int.jl:8 overwritten in module Main at none:1.
SYSTEM ERROR: Failed to report error to REPL frontendERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:ERROR (unhandled task failure): StackOverflowError:^C
^C
^C
^C
halp

@JeffBezanson
Copy link
Member Author

@mdcfrancis The problem with the old behavior is that "operators" were special-cased --- they were imported by default in a way that nothing else is. But what counts as an operator? It's a messy position to be in. Some packages want to define their own version of +, separate from the Base +, and have users do import FancyMath: + to use their + instead. IMO, there is no good reason for this to be impossible.

Adding methods to functions in other modules has global effects: the method will be visible in many places. I don't see why that's the safer default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants