Should mutable borrow of RasterBand require mutable borrow of it's Dataset?

[jdroenner]

Open Question from #144

[rmanoka]

This is a very interesting question. I don't have a good answer, but want to highlight a few points to discuss:

• Currently, even RasterBand::write needs only a &self ref; in contrast, Dataset::create_layer, and RasterBand::set_color_interpretation (from Add RasterBand's colour interpretation #144) do required an &mut self. This feels inconsistent, but I'm not sure which is the right way
• If we were to make RasterBand "writing" methods &mut self and also require it to have a &mut Dataset, then it would prevent a standard use-case of reading from one band and writing to another band of the same dataset. This seems too strict, and probably not what we want. I guess the question is: does GDAL write something on the Dataset, when modifying something about a RasterBand
• Just to understand the other extreme. Suppose we take the extreme case of interior mutability, and do not require &mut self for any of the methods, what could break? Are we violating any assumptions of the underlying C api?

Fortunately, Dataset is !Sync, and RasterBand is even !Send, so the mutations are still going to happen only in the same thread, and it is more a question of semantics. For example, suppose I have a RasterBand reference, and read pixel (0, 0) into a variable, then call a func foo(&RasterBand) which ends up writing to that pixel. Then, when the function returns, the value I read into the variable has been invalidated. Do we want to prevent this? Note that, it is anyway conceivable that someone else (either same thread, another thread, or even another process) could anyway have opened the same Dataset and modified things, so it was never a hard guarantee in the first place.

[rmanoka]

The standard library File is a good example to base our decision on. It is also a wrapper around a handle, and makes system calls for the actual operations. See in particular File::set_len which modifies the underlying file structure, but takes &self. This fact is also documented:

Note that this method alters the content of the underlying file, even though it takes &self rather than &mut self.

Thus, we should be in the clear to simply use &self in all methods of Dataset, RasterBand, and Layer.

Another option, is to use the compiler to enforce some semantics:

• All "read" methods take &self
• All "write" methods take &mut self
• The sub-objects: RasterBand and Layer keep &Dataset reference (as they currently do)

I like this as it may help avoid some pitfals at compile-time. It is also minimal conceptual change from current code-base, as we anyway use &mut self on some methods (like start_transaction). Also, this is at best a slap on the wrist anyway, as the same RasterBand could anyway be opened twice to get around the &mut self methods on it.

let ds = Dataset::open(...);
let band = ds.rasterband(1)?;
let same_band = ds.rasterband(1)?;

[jdroenner]

thanks for looking this up. This is a very good overview and File is a matching example. I guess we should do the read, write separation?