remove tuple indexing by non-integer

[oscardssmith]

Currently we don't throw an error for (1,2)[1.0] even though we really should. I'd be open to making this a deprectation warning rather than an error, if people think this might break actual people's code.

[fredrikekre]

No reason not to have a deprecation.

[jakobnissen]

Isn't this behaviour confusing though?

julia> key = 1.0;

julia> haskey((1,), key)
true

julia> (1,)[key]
ERROR

Relevant #42679

[JeffBezanson]

Uhoh

[JeffBezanson]

We should probably just fix haskey for arrays and tuples.

[jakobnissen]

I don't really have a dog in this fight, but I believe the only real way to fix this is by reverting #42679. The logic behind #42679 is simply flawed - keys(x) does not give you an exhaustive collection of all possible objects that is a valid key of x.

The problem in this PR is not limited to arrays and tuples. As long as #42679 is not reverted, it will appear whenever one of these two situations happen

• a == b, but only either a or b is a valid key (like this PR), in which case the generic fallback of haskey will be true, but indexing will error
• a != b, but both a and b are valid indices that refer to the same element. For example:

julia> v = [1]; i = CartesianIndex(1);

julia> haskey(v, i)
false

julia> v[i]
1

And these edge cases are endless - it's very easy to make a new type where one of the two situations above happen.

[JeffBezanson]

From triage: let's run pkgeval and plan to put in a deprecation for this. We should also revert #42679 (it's not in 1.7, so no big deal).

[oscardssmith]

@vtjnash anything else to do here (other than run nanosoldier?

[oscardssmith]

I just remembered that pkgeval isn't needed because I ended up deprecating rather than removing the "feature"

[vtjnash]

new tests are failing:

Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/tuple.jl:155
  Expression: getindex((), 1.0)
    Expected: BoundsError
      Thrown: ErrorException

once fixed, this LGTM

[oscardssmith]

@JeffBezanson can you look at the build error here? I don't understand why it's not working.

[simeonschaub]

The @deprecate macro needs at least 2 arguments, the old and the new expression. You might have to manually implement this using depwarn here.

[fredrikekre]

You might have to manually implement this using depwarn here.

This will give worse source information though? Why doesn't the macro work?

[oscardssmith]

I haven't managed to figure out a way to combine @deprecate and @eval that works.

[fredrikekre]

Why do you need to use @eval? This works:

@deprecate getindex(t::Tuple, i::Real) t[convert(Int, i)]

and gives

julia> (1,)[1.]
┌ Warning: `getindex(t::Tuple, i::Real)` is deprecated, use `t[convert(Int, i)]` instead.
│   caller = top-level scope at REPL[2]:1

[oscardssmith]

The problem is that changes the type of error of (1,)[0.]. We want it to be a BoundsError, but with that version, it is an ErrorException

[fredrikekre]

No?

julia> (1,)[0.]
┌ Warning: `getindex(t::Tuple, i::Real)` is deprecated, use `t[convert(Int, i)]` instead.
│   caller = top-level scope at REPL[2]:1
└ @ Core REPL[2]:1
ERROR: BoundsError: attempt to access Tuple{Int64} at index [0]

[oscardssmith]

huh, not sure what was happening then. Let's retry CI

[fredrikekre]

Probably the tests run with --depwarn=error (so the deprecation errors with an ErrorException).

[oscardssmith]

oh. That's it. I'll change the test.

[oscardssmith]

Can we merge this now? (Windows CI failure is unrelated, will be fixed by #44097)

[fredrikekre]

Probably these should be moved to https://github.com/JuliaLang/julia/blob/master/test/deprecation_exec.jl (which doesn't run with --depwarn=error).