Skip to content

Commit

Permalink
make == with non-comparable items an error
Browse files Browse the repository at this point in the history 
fix #9381
ref #15983
  • Loading branch information
@vtjnash
vtjnash committed May 20, 2016
1 parent ea10bdf commit 4533cc1
Show file tree
Hide file tree
Showing 16 changed files with 76 additions and 73 deletions.
12 changes: 6 additions & 6 deletions base/LineEdit.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1494,11 +1494,11 @@ activate(m::ModalInterface, s::MIState, termbuf, term::TextTerminal) =

commit_changes(t::UnixTerminal, termbuf) = write(t, takebuf_array(termbuf.out_stream))
function transition(f::Function, s::MIState, mode)
if mode == :abort
if mode === :abort
s.aborted = true
return
end
if mode == :reset
if mode === :reset
reset_state(s)
return
end
Expand Down Expand Up @@ -1599,16 +1599,16 @@ function prompt!(term, prompt, s = init_state(term, prompt))
warn(e)
state = :done
end
if state == :abort
if state === :abort
return buffer(s), false, false
elseif state == :done
elseif state === :done
return buffer(s), true, false
elseif state == :suspend
elseif state === :suspend
@unix_only begin
return buffer(s), true, true
end
else
@assert state == :ok
@assert state === :ok
end
end
finally
Expand Down
16 changes: 8 additions & 8 deletions base/REPL.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ end
function mode_idx(hist::REPLHistoryProvider, mode)
c = :julia
for (k,v) in hist.mode_mapping
v == mode && (c = k)
isequal(v, mode) && (c = k)
end
return c
end
Expand All @@ -387,7 +387,7 @@ function add_history(hist::REPLHistoryProvider, s)
isempty(strip(str)) && return
mode = mode_idx(hist, LineEdit.mode(s))
!isempty(hist.history) &&
mode == hist.modes[end] && str == hist.history[end] && return
isequal(mode, hist.modes[end]) && str == hist.history[end] && return
push!(hist.modes, mode)
push!(hist.history, str)
hist.history_file === nothing && return
Expand Down Expand Up @@ -457,13 +457,13 @@ function history_prev(s::LineEdit.MIState, hist::REPLHistoryProvider,
save_idx::Int = hist.cur_idx)
hist.last_idx = -1
m = history_move(s, hist, hist.cur_idx-1, save_idx)
if m == :ok
if m === :ok
LineEdit.move_input_start(s)
LineEdit.reset_key_repeats(s) do
LineEdit.move_line_end(s)
end
LineEdit.refresh_line(s)
elseif m == :skip
elseif m === :skip
hist.cur_idx -= 1
history_prev(s, hist, save_idx)
else
Expand All @@ -481,10 +481,10 @@ function history_next(s::LineEdit.MIState, hist::REPLHistoryProvider,
hist.last_idx = -1
end
m = history_move(s, hist, cur_idx+1, save_idx)
if m == :ok
if m === :ok
LineEdit.move_input_end(s)
LineEdit.refresh_line(s)
elseif m == :skip
elseif m === :skip
hist.cur_idx += 1
history_next(s, hist, save_idx)
else
Expand All @@ -508,7 +508,7 @@ function history_move_prefix(s::LineEdit.PrefixSearchState,
for idx in idxs
if (idx == max_idx) || (startswith(hist.history[idx], prefix) && (hist.history[idx] != cur_response || hist.modes[idx] != LineEdit.mode(s)))
m = history_move(s, hist, idx)
if m == :ok
if m === :ok
if idx == max_idx
# on resuming the in-progress edit, leave the cursor where the user last had it
elseif isempty(prefix)
Expand All @@ -520,7 +520,7 @@ function history_move_prefix(s::LineEdit.PrefixSearchState,
end
LineEdit.refresh_line(s)
return :ok
elseif m == :skip
elseif m === :skip
return history_move_prefix(s,hist,prefix,backwards,idx)
end
end
Expand Down
24 changes: 13 additions & 11 deletions base/array.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ end
function lexcmp(a::Array{UInt8,1}, b::Array{UInt8,1})
c = ccall(:memcmp, Int32, (Ptr{UInt8}, Ptr{UInt8}, UInt),
a, b, min(length(a),length(b)))
c < 0 ? -1 : c > 0 ? +1 : cmp(length(a),length(b))
return c < 0 ? -1 : c > 0 ? +1 : cmp(length(a),length(b))
end

function reverse(A::AbstractVector, s=1, n=length(A))
Expand All @@ -666,7 +666,7 @@ function reverse(A::AbstractVector, s=1, n=length(A))
for i = n+1:length(A)
B[i] = A[i]
end
B
return B
end
reverseind(a::AbstractVector, i::Integer) = length(a) + 1 - i

Expand All @@ -682,7 +682,7 @@ function reverse!(v::AbstractVector, s=1, n=length(v))
v[i], v[r] = v[r], v[i]
r -= 1
end
v
return v
end

function vcat{T}(arrays::Vector{T}...)
Expand Down Expand Up @@ -714,7 +714,7 @@ function hcat{T}(V::Vector{T}...)
throw(DimensionMismatch("vectors must have same lengths"))
end
end
[ V[j][i]::T for i=1:length(V[1]), j=1:length(V) ]
return [ V[j][i]::T for i=1:length(V[1]), j=1:length(V) ]
end


Expand All @@ -734,7 +734,7 @@ findfirst(A) = findnext(A, 1)
# returns the index of the next matching element
function findnext(A, v, start::Integer)
for i = start:length(A)
if A[i] == v
if isequal(A[i], v)
return i
end
end
Expand All @@ -758,25 +758,27 @@ function findprev(A, start::Integer)
for i = start:-1:1
A[i] != 0 && return i
end
0
return 0
end
findlast(A) = findprev(A, length(A))

# returns the index of the matching element, or 0 if no matching
function findprev(A, v, start::Integer)
for i = start:-1:1
A[i] == v && return i
isequal(A[i], v) && return i
end
0
return 0
end
findlast(A, v) = findprev(A, v, length(A))

# returns the index of the previous element for which the function returns true, or zero if it never does
function findprev(testf::Function, A, start::Integer)
for i = start:-1:1
testf(A[i]) && return i
if testf(A[i])
return i
end
end
0
return 0
end
findlast(testf::Function, A) = findprev(testf, A, length(A))

Expand All @@ -791,7 +793,7 @@ function find(testf::Function, A)
end
I = Array(Int, length(tmpI))
copy!(I, tmpI)
I
return I
end

function find(A)
Expand Down
12 changes: 10 additions & 2 deletions base/dates/io.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,17 @@ function DateFormat(f::AbstractString, locale::AbstractString="english")
last_offset = m.offset + width
end

tran = last_offset > endof(f) ? r"(?=\s|$)" : replace(f[last_offset:end], r"\\(.)", s"\1")
if !isempty(params)
slot = tran == "" ? FixedWidthSlot(params...) : DelimitedSlot(params..., tran)
if last_offset > endof(f)
slot = DelimitedSlot(params..., r"(?=\s|$)")
else
tran = replace(f[last_offset:end], r"\\(.)", s"\1")
if tran == ""
slot = FixedWidthSlot(params...)
else
slot = DelimitedSlot(params..., tran)
end
end
push!(slots,slot)
end

Expand Down
30 changes: 7 additions & 23 deletions base/dict.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,10 @@ const secret_table_token = :__c782dbf1cf4d6a2e5e3865d7e95634f2e09b5902__

haskey(d::Associative, k) = in(k,keys(d))

function in(p::Pair, a::Associative, valcmp=(==))
v = get(a,p[1],secret_table_token)
function in(p::Pair, a::Associative, valcmp=isequal)
v = get(a, p[1], secret_table_token)
if !is(v, secret_table_token)
if valcmp === is
is(v, p[2]) && return true
elseif valcmp === (==)
==(v, p[2]) && return true
elseif valcmp === isequal
isequal(v, p[2]) && return true
else
valcmp(v, p[2]) && return true
end
valcmp(v, p[2]) && return true
end
return false
end
Expand All @@ -30,7 +22,7 @@ end

function summary(t::Associative)
n = length(t)
string(typeof(t), " with ", n, (n==1 ? " entry" : " entries"))
return string(typeof(t), " with ", n, (n==1 ? " entry" : " entries"))
end

show{K,V}(io::IO, t::Associative{K,V}) = showdict(io, t; compact = true)
Expand Down Expand Up @@ -953,19 +945,11 @@ ImmutableDict
ImmutableDict{K,V}(KV::Pair{K,V}) = ImmutableDict{K,V}(KV[1], KV[2])
ImmutableDict{K,V}(t::ImmutableDict{K,V}, KV::Pair) = ImmutableDict{K,V}(t, KV[1], KV[2])

function in(key_value::Pair, dict::ImmutableDict, valcmp=(==))
function in(key_value::Pair, dict::ImmutableDict, valcmp=isequal)
key, value = key_value
while isdefined(dict, :parent)
if dict.key == key
if valcmp === is
is(value, dict.value) && return true
elseif valcmp === (==)
==(value, dict.value) && return true
elseif valcmp === isequal
isequal(value, dict.value) && return true
else
valcmp(value, dict.value) && return true
end
if isequal(dict.key, key)
valcmp(value, dict.value) && return true
end
dict = dict.parent
end
Expand Down
1 change: 1 addition & 0 deletions base/exports.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export
Irrational,
Matrix,
MergeSort,
NotComparableError,

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman May 20, 2016

Contributor

should this be grouped with other exception types? I think there's also a table of them in the docs

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan May 20, 2016

Member

Also, maybe IncomparableError would be a nicer name?

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman May 20, 2016

Contributor

I prefer NotComparable, otherwise it sounds like an adjective describing the error itself
NTuple,
Nullable,
ObjectIdDict,
Expand Down
2 changes: 1 addition & 1 deletion base/inference.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3152,7 +3152,7 @@ end
# boundscheck context in the method body
function inbounds_meta_elim_pass!(code::Array{Any,1})
if findfirst(x -> isa(x, Expr) &&
((x.head === :inbounds && x.args[1] == true) || x.head === :boundscheck),
((x.head === :inbounds && x.args[1] === true) || x.head === :boundscheck),
code) == 0
filter!(x -> !(isa(x, Expr) && x.head === :inbounds), code)
end
Expand Down
4 changes: 2 additions & 2 deletions base/multidimensional.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ end
end
@generated function _unsafe_setindex!(B::BitArray, X::Union{BitArray,Array}, I0::Union{Colon,UnitRange{Int}}, I::Union{Int,UnitRange{Int},Colon}...)
N = length(I)
rangeexp = [I[d] == Colon ? :(1:size(B, $(d+1))) : :(I[$d]) for d = 1:N]
rangeexp = [I[d] === Colon ? :(1:size(B, $(d+1))) : :(I[$d]) for d = 1:N]
quote
idxlens = @ncall $N index_lengths B I0 d->I[d]
@ncall $N setindex_shape_check X idxlens[1] d->idxlens[d+1]
Expand Down Expand Up @@ -667,7 +667,7 @@ end
end
@generated function _unsafe_setindex!(B::BitArray, x, I0::Union{Colon,UnitRange{Int}}, I::Union{Int,UnitRange{Int},Colon}...)
N = length(I)
rangeexp = [I[d] == Colon ? :(1:size(B, $(d+1))) : :(I[$d]) for d = 1:N]
rangeexp = [I[d] === Colon ? :(1:size(B, $(d+1))) : :(I[$d]) for d = 1:N]
quote
y = Bool(x)
idxlens = @ncall $N index_lengths B I0 d->I[d]
Expand Down
12 changes: 9 additions & 3 deletions base/operators.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ supertype(T::DataType) = T.super

## generic comparison ##

==(x,y) = x === y

isequal(x, y) = x == y
=={T}(x::T, y::T) = x === y
immutable NotComparableError <: Exception end
const NotComparable = NotComparableError()
==(x::ANY, y::ANY) = NotComparable

This comment has been minimized.

Sign in to view

Copy link
@kmsquire

kmsquire May 21, 2016

Member

Should this actually throw the error?
!(e::NotComparableError) = throw(e)
# immutable NotComparableError; a; b; end
# ==(x::ANY, y::ANY) = NotComparableError(x, y)

isequal(x, y) = (x == y) === true

This comment has been minimized.

Sign in to view

Copy link
@kmsquire

kmsquire May 21, 2016

Member

I guess I see here that you depend on not throwing above... though it still seems sketchy to me to return an error, rather than throw one.
isequal(x::AbstractFloat, y::AbstractFloat) = (isnan(x) & isnan(y)) | (signbit(x) == signbit(y)) & (x == y)
isequal(x::Real, y::AbstractFloat) = (isnan(x) & isnan(y)) | (signbit(x) == signbit(y)) & (x == y)
isequal(x::AbstractFloat, y::Real ) = (isnan(x) & isnan(y)) | (signbit(x) == signbit(y)) & (x == y)
Expand Down
4 changes: 2 additions & 2 deletions base/parse.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,11 @@ function parse(str::AbstractString, pos::Int; greedy::Bool=true, raise::Bool=tru
if raise && isa(ex,Expr) && is(ex.head,:error)
throw(ParseError(ex.args[1]))
end
if ex == ()
if ex === ()
raise && throw(ParseError("end of input"))
ex = Expr(:error, "end of input")
end
ex, pos+1 # C is zero-based, Julia is 1-based
return ex, pos+1 # C is zero-based, Julia is 1-based
end

function parse(str::AbstractString; raise::Bool=true)
Expand Down
6 changes: 3 additions & 3 deletions base/process.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -704,19 +704,19 @@ function arg_gen(head, tail...)
for h = head, t = tail
push!(vals, cstr(string(h,t)))
end
vals
return vals
end

function cmd_gen(parsed)
args = String[]
for arg in parsed
append!(args, arg_gen(arg...))
end
Cmd(args)
return Cmd(args)
end

macro cmd(str)
:(cmd_gen($(shell_parse(str)[1])))
return :(cmd_gen($(shell_parse(str)[1])))
end

wait(x::Process) = if !process_exited(x); stream_wait(x, x.exitnotify); end
Expand Down
2 changes: 1 addition & 1 deletion base/reduce.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ all(f::typeof(identity), itr) =

## in & contains

in(x, itr) = any(Predicate(y -> y == x), itr)
in(x, itr) = any(Predicate(y -> isequal(y, x)), itr)

const ∈ = in
∉(x, itr)=!∈(x, itr)
Expand Down
6 changes: 3 additions & 3 deletions base/show.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ function show_unquoted(io::IO, ex::Expr, indent::Int, prec::Int)
end

# scalar multiplication (i.e. "100x")
if (func == :(*) &&
if (func === :* &&
length(func_args)==2 && isa(func_args[1], Real) && isa(func_args[2], Symbol))
if func_prec <= prec
show_enclosed_list(io, '(', func_args, "", ')', indent, func_prec)
Expand Down Expand Up @@ -923,8 +923,8 @@ function show_unquoted(io::IO, ex::Expr, indent::Int, prec::Int)
end

function ismodulecall(ex::Expr)
ex.head == :call && (ex.args[1] == GlobalRef(Base,:getfield) ||
ex.args[1] == GlobalRef(Core,:getfield)) &&
return ex.head == :call && (ex.args[1] === GlobalRef(Base,:getfield) ||
ex.args[1] === GlobalRef(Core,:getfield)) &&
isa(ex.args[2], Symbol) &&
isdefined(current_module(), ex.args[2]) &&
isa(getfield(current_module(), ex.args[2]), Module)
Expand Down
Loading

6 comments on commit 4533cc1

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised by how non-disruptive this change is in Base Julia. I rather like it.

@kmsquire
Copy link
Member

Choose a reason for hiding this comment

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

This will break some things for users of Match.jl--at least, the assumption there is that everything is comparable. 😦

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

As for #9381, I'm still worried about 0.0 in A not finding -0.0. I think that's much more serious than cases involving NaN.

@kmsquire
Copy link
Member

Choose a reason for hiding this comment

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

This will break some things for users of Match.jl--at least, the assumption there is that everything is comparable.

Following up on this... suppose I want to be able to return false for things that are not comparable. What would be the best way forward? Redefine

==(x,y) = x === y
isequal(x, y) = x == y

in Match.jl?

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry all, I've deleted this branch to reduce confusion. I wasn't actually trying to propose doing this, but was just getting an idea of the scope of #9381 (though having an idea of the Match.ji use case is helpful)

@kmsquire
Copy link
Member

@kmsquire kmsquire commented on 4533cc1 May 21, 2016 via email

Choose a reason for hiding this comment

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

Please sign in to comment.