A 'close' annotation to restrict the bindings accessible by a block

[ExpHP]

cc rust-lang/rfcs#2496 (@bpsuntrup @oli-obk @burdges)

It would be nice to have a way to just restrict the bindings accessible by a block of code, kinda like one of the early planned features of JAI. @oli-obk suggested this could be done as a clippy lint, which sounds neat.

I'm opening this issue so we can bikeshed the syntax, discuss limitations, and possibly implement it (or to give people a chance to object to it being implemented in clippy).

Scattered thoughts/discussion starters in the next comment.

[ExpHP]

Strawmen

Here's some samples from @bpsuntrup's original idea (as a language feature):

close (&args, &mut to, &pass) {
   // enclosed logic, assured not to touch anything but args, to and pass
}

// (with a return type annotation)
let a: i32 = close (&args, &mut to, &pass) -> i32 {
   // enclosed logic, assured not to touch anything but args, to and pass
   to = args + pass;
   let some_int = 5 - args;
   some_int
}

Here's the example @oli-obk posted of a possible clippy lint:

#[clippy::close(mut x, move y)]
{
     // modifying x and y possible within block. Everything else can only be read
    // can modify Cell/Mutex though
}

I mostly like the above, although I'd also like to require that immutable bindings are specified too, as this is information I find useful (and then there can be warnings on things listed but not used!)

#[clippy::close(mut x, move y, z)]
{
    // - only x, y, and z can be used
    // - lint on any that aren't used. (or on unused mut for x)
}

Questions

• Is @oli-obk's syntax valid (yet)?
  • It's not a valid meta, but I recall there were plans to make the compiler support arbitrary token streams eventually... did this happen?
• Do we lint on mentioned but unused bindings?
• Should move imply mutability is allowed?
  • I think so. Notably, rustc's own borrowck errors will still require the original binding to be mut (or for it to be moved into an rvalue).
• Do we ensure that move bindings are moved?
• Do we lint against move for Copy types?
• What syntax should be used to specify a uninitialized immutable binding that is initialized by the block?

  // this seems misleading
  let x;
  #[clippy::close(x)] {
      x = 3;
  }
  
  // I don't mind this, but something feels odd about using mut
  // on an immutable binding
  let x;
  #[clippy::close(mut x)] {
      x = 3;
  }

• Do we want type annotations like in @bpsuntrup's idea?
  • I'm guessing it could not be used by type inference, so there's no point.
  • Even if there was a point, it could be a separate annotation

[bpsuntrup]

In response to @ExpHP ...

• Do we lint on mentioned but unused bindings?
  • This is a good idea.
• Should move imply mutability is allowed?
  • Please, no. That seems to defeat the purpose of immutability. Also, I think it's inconsistent with how other closures behave.
• Do we ensure that move bindings are moved?
  • Yes, because I think this would be consistent with how regular closures behave
• Do we lint against move for Copy types?
  • Probably not.
• What syntax should be used to specify a uninitialized immutable binding that is initialized by the block?
  • This is a very good question. I don't feel like I can make a smart answer on it. I kind of agree with your comments, though.
• Do we want type annotations like in @bpsuntrup's idea?
  • I think if inferences can be made, that's great. Otherwise I think it would be nice to allow type annotations, at least.

Really good questions. I don't know how to answer the first one.

[burdges]

This is correct now:

fn foo(x: u8) {
    // x is immutable here
    fn bar(mut x: u8) {
        // x is mutable here
    }
    bar(x)
}

[ExpHP]

• Do we ensure that move bindings are moved?
  • Yes, because I think this would be consistent with how regular closures behave

So actually, I used to have two questions here, which were more specific:

• Should move automatically move a binding (e.g. by automatically inserting let x = {x};), like in closures?
  • I'm guessing clippy probably is not allowed to even do this? (or chooses not to, out of principle?)
  • If not, maybe it should just ensure that it is moved with a warning?

After I edited it, I only meant the second case (that it should issue a warning if the user doesn't move it). But I guess it's still worth asking what the limitations of a clippy lint's powers actually are.

Please, no. That seems to defeat the purpose of immutability. Also, I think it's inconsistent with how other closures behave.

Similarly to the above, my feelings on this depend on whether or not clippy is allowed to modify the AST or MIR. If it isn't (or doesn't), then the answer to this one is that "it very clearly doesn't matter what clippy does:"

let x = vec![2, 4, 2];

#[clippy::close(move x)] {
    x.sort(); // regardless of what clippy does, if no modification is made
              // to the AST/MIR then rustc itself will reject this.
}

That said, even if clippy is allowed to modify the code, I'd rather see

let mut x = vec![2, 4, 2];
#[clippy::close(move x)] { ... }

//------------
// rather than either

let x = vec![2, 4, 2];
#[clippy::close(move mut x)] { ... }

// or

let mut x = vec![2, 4, 2];
#[clippy::close(move mut x)] { ... }

[oli-obk]

But I guess it's still worth asking what the limitations of a clippy lint's powers actually are.

Clippy can only detect when something doesn't match expectations. It cannot change the code. So yes, it would lint if move x is declared, but x is never moved out of.

let mut x = vec![2, 4, 2];

#[clippy::close(move x)] {
    x.sort(); // clippy warning: `x` marked as `move` but not `mut`, even though it's being mutated
}

on the other hand

let x = vec![2, 4, 2];

#[clippy::close(move x)] {
    let mut x = x;
    x.sort(); // totally fine
}

[ExpHP]

Thanks for clarifying!

With that, I'll also clarify my feelings on this one last point that also depended on interefering with compilation:

• Do we want type annotations like in @bpsuntrup's idea?
  • I think if inferences can be made, that's great. Otherwise I think it would be nice to allow type annotations, at least.

So, since clippy won't be able to use the annotation to tell the compiler about the type of something, I don't feel that there is anything useful that could be done with this. Clippy will be able to warn if the annotated type does not match the true type, but it won't be able to help you do anything about it. Picture the following:

(note: the comments only describe my best guesses about what would happen in each case)

// Clippy CAN warn about a type mismatch here...
// ...but how would it help the user?
// (no doubt, any code containing this probably has another type error somewhere)
let x = #[clippy::close() -> Vec<i32>] { 0 };

// The following will unintuitively produce an error.
// Clippy will know that you want a Vec, but rustc won't.
let mut x = #[clippy::close() -> Vec<_>] { (0..10).collect() };
x.sort();  // error: type annotations needed for `x`

// Here's an interesting case; suppose that nothing else constrains the type of x.
// Then the type of x will be defaulted to i32, and clippy will report a mismatched type,
// but the program will compile.  So this is a case where the lint actually accomplishes
// something... but is it helpful?
let x = #[clippy::close() -> i64] { 0 };
println!("{}", x);

These simple examples can be adjusted to something more useful by putting the type annotation directly on x:

// now the type of x actually IS i64
let x: i64 = #[clippy::close()] { 0 };
println!("{}", x);

And I get the feeling that this should also be the case in any real usage of #[clippy::close]; this is a feature that I envision will mostly be used on blocks that are too large to be embedded in a more complicated expression (0.75+ screenfuls). Do you disagree, @bpsuntrup?

[bpsuntrup]

That all seems sound to me, @ExpHP, although I should probably step back and disclose that I don't think I'm qualified to have a strong opinion about what would be the most practical. I think we're on the same page about how we envision the feature will be used, though.

[ExpHP]

I tried to look into this and quickly ran into a possible implementation challenge, for which I created #2916. Long story short, it does not appear to me that there is currently any reliable way for clippy to identify that all of these xs refer to the same variable:

let x = vec![1, 2];
if true {
    println!("{:?}", x.last());
    (|| {
        x.sort();
    })();
}

which I guess really isn't a problem for most lints as they can just give up in cases of uncertainty, but for a feature like this, I would really want the compiler's warnings to be precise. (otherwise, how can I trust the annotations I have written?)

---

Maybe in the interim, a reduction in the scope of the lint is necessary; i.e. to pick some subset of the warnings that still provide some meaningful information even if there are some false negatives.

@oli-obk's original suggestion might be easier to do (perhaps he foresaw these technical difficulties! 😉):

• allow any variable to be read
• make an effort to warn on mutations of something not marked mut, or moves of something not marked move, but with the understanding that some mutations may slip past it.
• no warnings about unused mut or unused move annotations for now.

I think this should be a lot easier to implement, though I'm not sure if I would find it very useful.