We're trying to add support for lists to hexchat-unsafe-plugin. This is what we have ended up with: commit
The main thing is lists, tho we wouldn't mind feedback on get_prefs or any of the rest of the stuff either. We also understand we're using Iterator slightly wrong, but it's probably fine.
Ah right you could leak the Entries and... wait no you can't use the Fields after the ValidContext is expired or &mut'ed, so it still works...
Hmm...
We think we can even add extra APIs to ValidContext this way! So that's pretty cool.
Currently, it's impossible to retrieve any entries at all. Entries::next() increments valid before returning a Fields with the previous id, so all attempts to access any field will panic on the assert_eq!.
Ugh. Wish we could actually run-test these...
... oh wait, we can. it's gonna take some effort to get there, but...
You don't mean to keep Fields::valid public, right? It punches a hole through the safety mechanism.
use hexchat_unsafe_plugin::{hexchat_plugin, list::Contexts, Plugin, PluginHandle};
use std::pin::Pin;
#[derive(Default)]
struct MyPlugin;
unsafe impl<'ph> Plugin<'ph> for MyPlugin {
fn init(self: Pin<&mut Self>, ph: &mut PluginHandle<'ph>, _: &str, _: Option<&str>) -> bool {
let _ = ph.with_context(&ph.get_context(), |ctx| {
let entry = ctx.list(&Contexts).next().unwrap();
entry.valid.set(0);
println!("{:?}", entry.name());
});
true
}
}
hexchat_plugin!('ph, MyPlugin);
ouch. no, those were meant to be pub(crate). >.<
it might work now?
It works for simple examples like the for loop. But it fails in many other cases, such as ctx.list(...).next().unwrap(), or ctx.list(...).map(...). One way to fix this would be to allow access to the data in a lifetime-restricted callback, then iterate over its outputs (i.e., implicitly map). This way, we're working with the lifetime system instead of fighting against it. As a proof of concept, it compiles just fine, and allows us to get rid of the id and valid fields:
diff --git a/src/lib.rs b/src/lib.rs
index cf04809..29240c5 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -379,8 +379,6 @@ mod valid_context {
// available through get_info.
list: ptr::null_mut(),
_t: &CONTEXTS,
- id: 0,
- valid: Default::default(),
},
_hidden: (),
}
@@ -1569,13 +1567,14 @@ impl<'a, 'ph: 'a> ValidContext<'a, 'ph> {
///
/// fn print_contexts(ph: &mut PluginHandle<'_>) {
/// ph.ensure_valid_context(|ph| {
- /// for context in ph.list(&Contexts) {
- /// write!(ph, "{}", context.name().unwrap());
+ /// for name in ph.list(&Contexts, |context| context.name()) {
+ /// write!(ph, "{}", name.unwrap());
/// }
/// })
/// }
/// ```
- pub fn list<T: list::List>(&'a self, t: &'a T) -> list::Entries<'a, 'ph, T> {
+ pub fn list<'b, T: list::List, U, F>(&'b self, t: &'b T, f: F) -> list::Entries<'b, 'ph, T, F>
+ where F: FnMut(list::Fields<'_, 'ph, T>) -> U {
let ph = &self.ph;
let list = CString::new(&*t.name()).unwrap();
let list = unsafe {
@@ -1585,7 +1584,7 @@ impl<'a, 'ph: 'a> ValidContext<'a, 'ph> {
context: self.ph,
list: list,
t: t,
- valid: Default::default(),
+ f: f,
}
}
diff --git a/src/list.rs b/src/list.rs
index 5d40e5a..ff320ae 100644
--- a/src/list.rs
+++ b/src/list.rs
@@ -17,9 +17,7 @@
//! List support module.
use std::borrow::Cow;
-use std::cell::Cell;
use std::ffi::CStr;
-use std::rc::Rc;
use crate::Context;
use crate::wrap_context;
@@ -52,11 +50,11 @@ pub struct Notify;
pub struct Users;
/// Entries.
-pub struct Entries<'a, 'ph, T> {
+pub struct Entries<'a, 'ph, T, F> {
pub(crate) context: &'a crate::PluginHandle<'ph>,
pub(crate) list: *mut crate::internals::HexchatList,
pub(crate) t: &'a T,
- pub(crate) valid: Option<Rc<Cell<usize>>>,
+ pub(crate) f: F,
}
/// Fields.
@@ -64,8 +62,6 @@ pub struct Fields<'a, 'ph, T> {
pub(crate) context: &'a crate::PluginHandle<'ph>,
pub(crate) list: *mut crate::internals::HexchatList,
pub(crate) _t: &'a T,
- pub(crate) id: usize,
- pub(crate) valid: Rc<Cell<usize>>,
}
// impls
@@ -96,47 +92,31 @@ impl List for Users {
fn name(&self) -> Cow<'static, str> { "users".into() }
}
-impl<'a, 'ph, T> Drop for Entries<'a, 'ph, T> {
+impl<'a, 'ph, T, F> Drop for Entries<'a, 'ph, T, F> {
fn drop(&mut self) {
let ph = &self.context;
- if let Some(ref valid) = self.valid {
- valid.set(usize::MAX);
- }
unsafe {
ph_call!(hexchat_list_free(ph, self.list));
}
}
}
-impl<'a, 'ph, T> Iterator for Entries<'a, 'ph, T> {
- type Item = Fields<'a, 'ph, T>;
+impl<'a, 'ph, T, U, F> Iterator for Entries<'a, 'ph, T, F>
+where F: FnMut(Fields<'_, 'ph, T>) -> U {
+ type Item = U;
fn next(&mut self) -> Option<Self::Item> {
let ph = &self.context;
if unsafe {
ph_call!(hexchat_list_next(ph, self.list))
} != 0 {
- let valid = if let Some(ref valid) = self.valid {
- debug_assert!(valid.get() < usize::MAX);
- valid.set(valid.get() + 1);
- Rc::clone(valid)
- } else {
- let valid: Rc<Cell<usize>> = Default::default();
- self.valid = Some(Rc::clone(&valid));
- valid
- };
- let id = valid.get();
- Some(Fields {
+ let fields = Fields {
context: self.context,
list: self.list,
_t: self.t,
- valid: valid,
- id: id,
- })
+ };
+ Some((self.f)(fields))
} else {
- if let Some(ref valid) = self.valid {
- valid.set(usize::MAX);
- }
None
}
}
@@ -145,10 +125,6 @@ impl<'a, 'ph, T> Iterator for Entries<'a, 'ph, T> {
macro_rules! field {
($(#[$m:meta])* $f:ident, unsafe {$n:expr}, $r:ty, $ph:ident, $c:ident, $($conv:tt)+) => {
$(#[$m])* pub fn $f(&self) -> $r {
- assert_eq!(
- self.id, self.valid.get(),
- "instances of Fields are invalidated by next()",
- );
let $ph = &self.context;
const NAME: &'static CStr = unsafe {
CStr::from_bytes_with_nul_unchecked(
Uh does it fail unsoundly tho? (also wait, .map() fails?)
My apologies, I had accidentally tested the map on the older non-functioning version. By failing, I mean that it panics on common Iterator operations (such as next() or last() or nth() or anything else that consumes the iterator).
Hmm, wonder if we can workaround the issue without losing most of Iterator's methods...
We know this is a badly-behaved Iterator but, eh. Besides, being able to work with multiple lists at a time is kinda nice. This is basically the StreamingIterator problem, we guess.
As another alternative, instead of Entries implementing Iterator, it could define its own next() and map() methods (and perhaps for_each()) with the appropriate lifetimes:
impl<'a, 'ph, T> Entries<'a, 'ph, T> {
pub fn next(&mut self) -> Option<Fields<'_, 'ph, T>> {
todo!()
}
pub fn for_each<F>(mut self, mut f: F)
where
F: FnMut(Fields<'_, 'ph, T>),
{
while let Some(entry) = self.next() {
f(entry);
}
}
pub fn map<U, F>(self, f: F) -> EntriesMap<'a, 'ph, T, F>
where
F: FnMut(Fields<'_, 'ph, T>) -> U,
{
EntriesMap { entries: self, f }
}
}
pub struct EntriesMap<'a, 'ph, T, F> {
entries: Entries<'a, 'ph, T>,
f: F,
}
impl<'a, 'ph, T, U, F> Iterator for EntriesMap<'a, 'ph, T, F>
where
F: FnMut(Fields<'_, 'ph, T>) -> U,
{
type Item = U;
fn next(&mut self) -> Option<Self::Item> {
self.entries.next().map(&mut self.f)
}
}
This would allow both direct access to the Fields values and transformation into a regular iterator.
we guess. tho we really wanted it to work with for loops...
for "proper" support, we think we're gonna have to wait for StreamingIterator/GATs. which is probably gonna take a few years. but in the mean time, as we said in the OP, we don't mind having a "wrong" Iterator.
tho we might wanna wrap the Entries with an Rc or something and clone that Rc into the Fields so it can also support ctx.list(...).next().unwrap(). but we believe we can do that without breaking the current API, so it's not really blocking the 2.2 release...
thoughts?
With an immediate-map implementation, for loops are perfectly possible. Either with my former suggestion:
for name in ph.list(&Contexts, |ctx| ctx.name()) {
write!(ph, "{}", name.unwrap());
}
Or with my latter suggestion:
for name in ph.list(&Contexts).map(|ctx| ctx.name()) {
write!(ph, "{}", name.unwrap());
}
// or equivalently:
ph.list(&Contexts).for_each(|context| {
write!(ph, "{}", context.name().unwrap());
});
Even with next() alone, a while let loop suffices:
let mut list = ph.list(&Contexts);
while let Some(context) = list.next() {
write!(ph, "{}", context.name().unwrap());
}
IMO, any of these would be preferable to a wonky Iterator, at least until a GAT-based lending iterator is available (which could take quite a while).
Hmm.
Does that one work?
The first should work with the former approach (modifying ValidContext::list()), and the other three should work with the latter approach (adding methods to Entries).
Does the new commit work?
Yes, the Entries::next() function works. (Note, though, that the where 'a: 'b bound does nothing at all. The &'b mut self reference introduces the implied bound Entries<'a, 'ph, T>: 'b, which in turn implies that 'a: 'b.)
Meanwhile, I noticed an issue which causes unsoundness. In src/list.rs, the field_*! macros can optionally take a string literal wrapped in unsafe {}. The macros then pass this string literal to CStr::from_bytes_with_nul_unchecked(). However, none of the string literals end with a \0 byte, so the CStr continues past the end. In practice, this results in an invalid field name, resulting in HexChat returning -1 or NULL as appropriate. As an experiment, I added a line to hexchat_list_int to print out the string it receives, and the field name is followed by garbage:
"idassertion failed: 0 < pointee_size && pointee_size <= isize::MAX as usize/rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ptr/const_ptr.rsMAX is not 0/rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/rc.rs"
So these string literals should probably end in \0.
hmm...
is there any sort of warning for redundant bounds like this?
we don't know of any. probably nobody cares...
well, the NULs should be fixed now. but at this point we feel like we're using you as almost a compiler. we really should finish that mock.rs tbh...
Not really, unfortunately. AFAICT this kind of lifetime bound is always evaluated when the function is called, not when it is declared, so neither rustc nor Clippy evaluates whether it is trivial.
Well, we're kinda sorta done with this whole thing for now. We hope the released v2.2.0 and v2.3.0 aren't unsound. Thanks for all the help!