Borrow linter

[Desdaemon]

(Just an idea, it doesn't have to go anywhere!)

While RustOpaque has been available for a while, the ergonomics of using it hasn't actually improved. In some cases you still need to reach out for ..move=, and improper usage can lead to deadlocks which are annoying to detect/fix. Rust is known for its strict rules which allow experienced developers to avoid costly clones, but these rules become harder to enforce when using bindings in Dart.

The most obvious problem is RwLock, which requires that writers and readers do not simultaneously access the data. For example, this code will always fail at runtime:

class Foo {
  // Implemented in Rust as "fn foo(&mut self, another: &Foo)"
  void doSomething(Foo another);
}

Foo foo;
foo.doSomething(foo);
// &mut self acquires write lock, succeeds
// 'another' acquires read lock, deadlocks because write lock is still active

While this code looks silly and one can perhaps safeguard against this kind of behavior with documentation, it is better still if linters can help detect statically that this code would not do what the user intends. In this instance, if a warning were to be emitted from this usage of doSomething, it could prompt the user to call clone as needed, or change the parameters to suit their purposes.

Proposal

Review the defaults of opaque passing, and enforce these rules via a lite version of the borrow checker.

Suppose we have three new attributes, each denoting a requirement placed on the usage of a RustOpaque:

• @read is the most lenient: most methods take &self so this can be the default for methods, and for parameters it denotes readonly access to the value.
• @write is Rust's mut: both methods taking &mut self and parameters taking a mutable reference will need to be annotated as such.
• @consume is Rust's move: this is the default for parameters that take values, and methods taking self will need to have this annotation as well.

An example declaration:

class Foo {
  // No annotation, so takes &self.
  String get name;
  // fn update(&mut self, other: &Foo);
  @write
  void update(@read Foo other);
  // fn into_bar(self);
  @consume
  Bar get intoBar;
}

We can implement the basic rules of the borrow checker using these attributes, for example:

• Once an opaque is consumed, it may not be used until it's reassigned to something alive.
• Once a read lock is acquired, it may not be consumed nor used with @write methods.
• Once a write lock is acquired, it may not be consumed nor its lock be reacquired until the end of the function call.
• If users pass these RustOpaques to other functions Dart-side, they can also opt to use these annotations to accurately model their usage, which will be picked up for analysis by the linter.

[Desdaemon]

Though if I'm perfectly honest, this is just an excuse so that me (or anyone interested) can learn how to write a linter with all the attendant features.

[fzyzcjy]

Looks interesting! I will read carefully and think about it soon (hopefully today)

Btw (without reading the content, just see your last comment) for writing custom linter infra, IIRC there is https://github.com/invertase/dart_custom_lint (maybe also some others)

[fzyzcjy]

1. Proposal: Use try-lock instead of lock to avoid deadlock?

Some brainstorm: Suppose we use https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.try_read to replace read (and same for write). Then, deadlock will not happen, since it will immediately create an error.

There may be a drawback: What if the user really needs it to wait for some lock? However, I am not sure whether this will exist in real code (except for buggy code, surely). Dart is single-threaded (except when using another isolate, but that's another story), so it seems that we will not run into concurrent problems. What do you think?

2. Discussions about the linter

This looks quite interesting! Even if we use try-lock above and it works, this may still be useful, because it can provide static compile-time hints, instead of the runtime hints by try-lock.

By the way, as for ..move, it seems that rust-auto-opaque (instead of the explicit RustOpaque<T>) is used, thus it should already understand Something, &Something, &mut Something and automatically use move or borrow respectively.

Though if I'm perfectly honest, this is just an excuse so that me (or anyone interested) can learn how to write a linter with all the attendant features.

Agree, that looks quite interesting ;)

[Desdaemon]

As for whether to use try_lock, for one of my projects it was sufficient to use blocking read and try_write, since this would prevent write access e.g. when the read lock is still in scope, indicating user error. Sadly it can't tell which line is the offending read though.

[fzyzcjy]

I see.

sufficient to use blocking read and try_write

Not very sure, shall we use try_read + try_write or blocking-read + try_write? (the former seems more symmetric)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new issue.