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

Remove type-unlimited unary + and * #45320

Merged
merged 2 commits into from
May 17, 2022
Merged

Remove type-unlimited unary + and * #45320

merged 2 commits into from
May 17, 2022

Conversation

dkarrasch
Copy link
Member

Addresses #44564 (comment).

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@dkarrasch dkarrasch changed the title Remove type-unlimited + and * Remove type-unlimited unary + and * May 16, 2022
@bkamins
Copy link
Member

bkamins commented May 16, 2022

I am moving the discussion to this PR from #44564. CC @nalimilan

I'm very sorry for the breakage!

No problem - that is why I am reporting.


The point is that - is different than * and + because in - you multiply by UInt8(-1) which will error if something is undefined, while * and + silently go-through even when they do not make sense to be allowed.

I do not insist on this PR, but I would want to have a conscious decision that we want to allow these operations (in DataFrames.jl I strictly test if nonsense operations error - that is how I got to this PR 😄).

Specifically in Julia 1.7, since addition of NamedTuple in general is disallowed then also:

julia> +(NamedTuple())
ERROR: MethodError: no method matching +(::NamedTuple{(), Tuple{}})

while after your PR:

julia> +(NamedTuple())
NamedTuple()

The point is that, in general you call addition like +(args...) and in generic code you do not know how many args you get. Previously if all args were NamedTuple the code always errored. Now it errors for more than one arg, but for one arg it works.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC KristofferC merged commit 990b1f3 into master May 17, 2022
@KristofferC KristofferC deleted the dk/operators branch May 17, 2022 11:56
@@ -512,10 +512,10 @@ julia> identity("Well, what did you expect?")
"""
identity(@nospecialize x) = x

+(x) = x
+(x::Number) = x
-(x) = Int8(-1)*x
Copy link
Member

@StefanKarpinski StefanKarpinski May 26, 2022

Choose a reason for hiding this comment

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

Kind of seems like all of these other type-unlimited arithmetic fallbacks should also be done away with. Issue filed: #45463.

DilumAluthge added a commit that referenced this pull request May 28, 2022
DilumAluthge added a commit that referenced this pull request Jul 25, 2022
…) (#45489)

* Revert "Remove type-unlimited unary `+` and `*` (#45320)"

This reverts commit 990b1f3.

* Revert "Generalize or restrict a few basic operators (#44564)"

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl

Co-authored-by: Daniel Karrasch <[email protected]>
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
…and JuliaLang#45320) (JuliaLang#45489)

* Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)"

This reverts commit 990b1f3.

* Revert "Generalize or restrict a few basic operators (JuliaLang#44564)"

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl

Co-authored-by: Daniel Karrasch <[email protected]>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
…and JuliaLang#45320) (JuliaLang#45489)

* Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)"

This reverts commit 990b1f3.

* Revert "Generalize or restrict a few basic operators (JuliaLang#44564)"

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl

Co-authored-by: Daniel Karrasch <[email protected]>
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.

5 participants