lint: add internal mutability lint.

[huonw]

This picks up any use of types with internal mutability (containing
Unsafe) in function/method calls, since these may cause things to be
modified unexpectedly.

This lint is like the "heap memory" lints, in that using internally
mutable types is perfectly valid in many instances, but it is easy to
expose the compiler's internal knowledge for the cases when someone does
wish to keep track of everything being mutated.

[lilyball]

In IRC today I was strongly opposed to this lint. Thinking on it again, I can see why you might want to turn it on only for specific functions (as opposed to turning it on for a full module/crate, where I think it's overly broad and therefore useless).

But once you start considering turning this on for specific functions, it seems like a poor man's way of trying to make the function pure. Except it's not good enough for that, because it doesn't handle unsafe mutability (such as touching static mut variables), or explicit &mut mutability. Perhaps we would be better served by actually trying to re-introduce the idea of a pure function, which is not allowed to touch global state or mutate any of its arguments (either by taking &mut or by using internal mutability).

[huonw]

I think a reasonable usecase for this is turning it on for a library like libcollections, where they should be (for the most part) straightforward owned types. This will stop accidental/"lazy" use of Cell etc.

(If there something in that library that absolutely needs internal mutability, then it can turned on just for that type/module.)

[thestinger]

It's easy to remove a lint without breaking backwards compatibility (make it a no-op + warn) so I don't think there's harm in trying it. It might turn out to be useful because it's too restrictive and has too many false positives, but I don't think we'll ever find out if we don't try.

[lilyball]

@alexcrichton You tend to be a very slippery-slope kind of guy. What's your thought on the idea of just adding a lint to try it out?

[lilyball]

@huonw What would be the actual benefit of turning it on for something like libcollections? I don't see Cell as being a "lazy" thing to use. The only way to use it lazily is to not bother designing your API properly, and then realize you want mutation in a &self method, and opting to use Cell rather than go back to the API design board. But our libraries aren't built that way.

[huonw]

Yes, it's to force people to design their APIs properly (or at least think about them) when adding new data structures.

---

I've had to get the compiler to record ReachesUnsafe in addition to its current recording of InteriorUnsafe. cc @nikomatsakis

[pcwalton]

Agreed with @thestinger. Lints are cheap. Lots of folks like purity, so this is a good one. Add 'em at will.

[DaGenix]

Its currently off by default. My intuition would be that that means that it will see very little use which somewhat defeats the purpose of the lint. On the other hand, I don't think it should be on by default either since I would think it very strange if there is a lint that produces warning as soon as you use Cell, one of the basic Rust types (I'm saying its a basic type since its in libcore).

As mentioned by other comments, there are other forms of mutability that this won't touch - using a static mut or calling out to C code.

It doesn't feel to me like this really solves the currently awkward relationship between aliasability and mutability, at least when it comes to conveying useful information regarding programmer intent. Rust's story regarding mutability and programmer intent is currently very hazy, and I don't think this makes it significantly clearer.

I don't see the harm in adding the lint, but, I don't think its all that useful either.

[thestinger]

Its currently off by default. My intuition would be that that means that it will see very little use which somewhat defeats the purpose of the lint. On the other hand, I don't think it should be on by default either since I would think it very strange if there is a lint that produces warning as soon as you use Cell, one of the basic Rust types (I'm saying its a basic type since its in libcore).

Cell is included in libcore because it has no external dependencies, not because it's meant to be a commonly used type. It's code smell no matter where it's used, and a lint for it makes sense. It's sometimes necessary for the sake of performance or simplicity, but it remains ugly.

As mentioned by other comments, there are other forms of mutability that this won't touch - using a static mut or calling out to C code.

This lint allows disabling all types with inherited mutability. Using a mutable global variable is an external effect rather than mutation of a variable. It could be saving state in POSIX shared memory or an on-disk file rather than a global. If Rust ever gains an effects system, modifying a global or task-local variable will be an effect outside of the work the mutability system needs to worry about.

Venturing outside of inherited mutability always requires unsafe code, and breaking the fundamental rules by performing mutation without inherited mutability or a contained Unsafe<T> is undefined behaviour so this lint will catch 100% of non-inherited mutability for variables.

It doesn't feel to me like this really solves the currently awkward relationship between aliasability and mutability, at least when it comes to conveying useful information regarding programmer intent.

This "awkward" relationship is the foundation of what makes Rust memory safe. It will always be painful, and no amount of obfuscation / renaming is going to make the borrow checker errors fade into the background. The borrow checker is implemented on top of the underlying ownership-based mutability system, and not all of the checks are related to aliasing.

Rust's story regarding mutability and programmer intent is currently very hazy, and I don't think this makes it significantly clearer.

Rust's mutability system is well defined without anything hazy or confusing involved. The mut marker means a variable or reference can be assigned to, nothing more and nothing less. It means variable = 5 and *reference = 5 will compile. Most types inherit mutability from the owner, and some owners are variables. A mutable reference is simply observing the mutability of the owner, so it's still the same inherited mutability system.

It's not possible for a mutability system to take everything into account. It's trivial for "pure" Haskell code to spin in a loop consuming all of the system's memory, spinning up the CPU fan and heating up the room until the system grinds to a halt swapping to disk and finally the OOM killer is triggered and ends the misery of the Haskell program, perhaps after killing the user's Eclipse process.

The lack of perfection doesn't make something any less useful.

[huonw]

I thought of an even better way to do this: only warn if the type reaches at least one Unsafe via a pointer (i.e. not just inline in the value being passed to a function) since inline data cannot be visibly modified when passed by-value.

struct Inline {
    x: Unsafe<uint>
}

struct Pointer<'a> {
    y: &'a Unsafe<uint>
}

let inline: Inline = ...;
let ptr: Pointer = ...;

foo(&inline); // warn
foo(inline); // no warning: there's no way for `inline` itself to be visibly mutated

foo(&ptr); // warn
foo(ptr); // warn

This allows e.g. Cell and RefCell to be passed around by-value without a warning.

However, the compiler doesn't directly support computing this property, so I'll not implement it for now (unless someone thinks this is very much better than also warning for the foo(inline)-style uses, as the code does now).

[huonw]

(That could be improved even more, by modelling pointer ownership, e.g. passing any of Box<Cell<...>>, HashMap<K, Cell<...>> or Vec<Cell<...>> is fine without a warning. But doing this is even harder.)

[DaGenix]

@thestinger A function that calculates the first 10,000 digits of PI might also be freestanding, but that isn't sufficient for it to be included in libcore. I disagree that there is anything wrong with Cell. Its just a type with a safe interface that just happens to have an unsafe implementation. Depending on the problem to be solved, there may not be any reasonable way to avoid using it, so, I don't agree that it's a second class type that should have a warning emitted when its used.

If a type from the core library that is necessary to solve some problems needs to have a warning when it used because it breaks that mutability system, I think that's not a sign that there is something wrong with Cell (and friends), but that there is a problem with the mutability system that a lint won't fix.

As I said, I don't see the harm in adding the lint and I can see it as useful in some contexts, but, I don't see it as a solution to the bigger issues.

[thestinger]

@DaGenix: If Cell wasn't ugly, it wouldn't need Unsafe<T> in the type definition to make it well-defined. It's not possible to implement it with only unsafe code, the compiler has to be informed about the internal mutability. It is very much a second-class type (as is anything containing Unsafe<T> like Rc), and should be avoided whenever convoluted or poorly performing code isn't required instead.

Any usage of Unsafe<T> will cause certain optimizations to be disabled when Rust has type-based alias analysis. An &T reference can be assumed to be immutable if and only if T contains no Unsafe, so LLVM can be informed that it's a non-aliasing, read-only pointer. The definition of aliasing used by C99 and optimizing compilers is a memory dependency, not address equality.

[huonw]

Hm, so I actually ran this on real code rather than just what I conjured up for the test... i.e. libcollections: error: aborting due to 1617 previous errors. Trait objects, closures and generics are all considered to contain Unsafe so anything using any of those is warned about.

This was a nice idea, but requires significantly more implementation work. :(