deprecate `@inbounds` and change it to `@unsafe_inbounds`

[Suavesito-Olimpiada]

Reading Discussion on "Why I no longer recommend Julia", I came to the notion that @inbounds have the ability to corrupt memory or access unassign memory. As with the rest of Base I think it should be changed to @unsafe_inbounds.

To avoid breakage, a deprecation warning would be thrown, suggesting changing it to the new name. I can make the PR, but wanted to hear, what do you think?

[oscardssmith]

deprecating @inbounds would be massively breaking. Unless you are willing to go through and make the change to a few thousand libraries, I think this is a non-starter.

[Suavesito-Olimpiada]

I don't know the complete lexical used here, but by deprecation I meant it being usable yet, but just with a deprecation warning. 😅

I would be ready to go through as many packages as I could changing this, as my summer break is really near. 😛

[DNF2]

Is it possible to introduce @unsafe_inbounds, without deprecating @inbounds, but changing the manual and the docstring of @inbounds to say

@inbounds will be removed in a future version. Please use @unsafe_inbounds instead.

Matlab does this sort of thing, at least.

[ianatol]

The docstring for @inbounds already warns as follows, and personally I don't think that changing the name to @unsafe_inbounds will dissuade anyone from a risky usage more than this warning already does.

  │ Warning
  │
  │  Using @inbounds may return incorrect results/crashes/corruption for out-of-bounds indices. The user is
  │  responsible for checking it manually. Only use @inbounds when it is certain from the information locally
  │  available that all accesses are in bounds.

[Suavesito-Olimpiada]

That is true, but one person on Discourse said that didn't know about it being unsafe.

A nice design that Base follows is being explicit on a lot of things, one of them is marking functions as unsafe when they are

I think that design should be followed here. Mostly because of consistency, but also because it could be educational in occasions where people don't know about this being unsafe. It is not so much about dissuading people from using it.

[PallHaraldsson]

TL;DR disagree with "KristofferC added merge me and removed merge me" (the former part).

a) I'm not sure about changing this, then you would have two versions out there, @unsafe_inbounds (clearly unsafe, as in C/C++, just explicit about it) and @inbounds still unsafe, meaning the same but implying to some soething different.

b) And you also have ccall (and @ccall) which keep being unsafe. I think people will just have to learn the these idioms of the language. The user can turn off a) globally.

If some way of tackling OffsetArrays.jl warrants a new macro with different semantics, then I like to hear it (likely my proposal was flawed).

[Keno]

I think the best way forward here is to dramatically reduce our dependence on @inbounds - it really has gotten out of hands. We need to improve the compiler to make sure that "obvious" patterns like:

for i = 1:length(A)
A[i]
end

don't emit bounds checks anymore. LLVM has most of the info that's needed for this - somebody needs to look at why that's not firing.

Once we've done that, it'll be much easier to come up with safer versions of @inbounds, because the use across the ecosystem won't be as sprawling.

[Suavesito-Olimpiada]

the best way forward here is to dramatically reduce our dependence on @inbounds

I think you're right, @inbounds really feels like a hacky way to tell the compiler something that it should already know, at least for some types (Array).

somebody needs to look at why that's not firing

Lamentably, I don't speak compiler. :'V

safer versions of @inbounds

What would a safer version of inbounds looks like?

[KristofferC]

TL;DR disagree with "KristofferC added merge me and removed merge me" (the former part).

Huh? I fat fingered adding the label and then I removed it... It is also an issue, not a PR.

[bvdmitri]

@Keno the compiler to make sure that "obvious" patterns like

Why would you assume it is an "obvious" pattern? It implicitly assumes that indexing starts with 1, but no one tells that A has valid index 1. The whole point of the posted article is that the code you posted is unsafe with removed bounds checking. Can any compiler figure that out automatically?

[StefanKarpinski]

One option would be to make the current @inbounds a no-op, thereby disabling its many uses throughout the ecosystem, and introduce @unsafe_inbounds that does what the current macro does. That would not break anything since ignoring inbounds is always valid and anything that errors when ignoring it was already broken. It would allow the ecosystem to incrementally review uses of the macro and either:

1. leave them, implicitly saying they are no longer necessary,
2. delete them, explicitly saying they’re no longer necessary, or
3. change them to @unsafe_inbounds, explicitly saying they’re still necessary.

However, in order for this not to cause a widespread performance hit, it would be necessary to make sure the compiler can eliminate bounds checks on its own most of the time.

[Seelengrab]

I don't think @inbounds is ever going to go away completely, at the very least not while it's allowed to use any arbitrarily computed integers as indices. Consider this (admittedly contrived) example:

julia> using Base.Iterators

julia> function foo(A, n)
          s = 0
          idxs = 1:2
          checkbounds(Bool, A, idxs) || error("not inbounds")
          for idx in Iterators.filter(==(first(idxs)), flatten(first(repeated(idxs), length(idxs)*n)))
            s += A[idx] == 1
          end
          return s
       end
foo (generic function with 1 method)

All information required for this is right there, but if the compiler can ever figure out that this will always be inbounds, I'd be extremely impressed. At the very least, it would require formalizing the bounds checking interface further and have @inbounds require that (I'd be very much in favor of this!).

As such, renaming or otherwise deprecating the current version feels like trying to cure a symptom instead of getting to the root of the problem: users not respecting the warnings & caveats described in its documentation. A name won't change that, if the macro is otherwise useful to them. Heck, Base.@pure isn't even exported and explicitly says that its for internal use only. Still, people used it and complained when their code was broken due to misuse.

Once we've done that, it'll be much easier to come up with safer versions of @inbounds, because the use across the ecosystem won't be as sprawling.

Well unless people go back through old code and replace the @inbounds with that safer version, I think we're stuck with it. 🤷 We could of course also go through the ecosystem and fix erroneous uses today..

[oscardssmith]

@bvdmitri it's safe for the compiler to add the inbounds for types where it is valid (separate code gets compiled for different types).