Add null-check for function's documents

[kngwyu]

We have a feature that extracts Python's doc string from doc comments in proc-macro code.

In the related parts, I found all of the code assumes that the corresponding document is null-terminated and does not check that.
This can make it difficult to debug our proc-macro code when it produces not null-terminated doc by accident, so I added a check using CStr.

[sebpuetz]

I think this is a nice improvement, maybe we can also change the docs to take &str instead of &'static str and rely on CString::into_raw for this, too?

It might also make sense to change the name and doc fields of PyMethodDef to CString and ensure well-formedness in constructors and in setter methods. If we also provide getter methods, we can encapsulate these fields, too.

E.g. something like

impl PyMethodDef {
    fn set_name(&mut self, name: &str) -> Result<()> {
        let name = CString::new(name)?;
        self.name = name;
        Ok(())
    }
}

[kngwyu]

It might also make sense to change the name and doc fields of PyMethodDef to CString and ensure well-formedness in constructors and in setter methods.

It must be static and we cannot do so.

[sebpuetz]

We can allocate a String and leak it, thus creating our own &'static str or we can mirror what is done now for name and use CString::into_raw since this transfers ownership to the C-caller.

[kngwyu]

Probably we can, but it would require lots of refactoring.

[kngwyu]

Now I want to investigate some other solutions. E.g.,

pub struct PyMethodDef {
    pub ml_name: CString,
}

So I leave this PR unmerged for two or three days (it looks that I did too much OSS work this week and I have to do some research works as a student...)

[kngwyu]

UPDATED
Now the internal representations of ml_name and ml_doc are &'static CStr. I.e., they are static strings with terminating null bytes, for efficeincy and null safety.
However, this change makes the PyMethodDef constructor not-const, and now Pyo3MethodsInventory~ has Vec<PyMethodDef> instead of &'static [PyMethodDef],

[kngwyu]

🤔

[kngwyu]

@davidhewitt
If you're OK to use Vec instead of &'static [], I will merge this PR.

[kngwyu]

And I'm sorry that this PR contains many changes to make fields of Py~Def types private, but I believe it's not good to keep them public.

[davidhewitt]

LGTM; I think Vec is fine here. Maybe slightly less optimal but it's one-time-setup anyway and the safety is a huge plus. Maybe one day it'll be possible to do this with const fn 🤷

I guess with the removal of public fields this is technically a breaking change, so maybe let's merge this and abandon the idea of making a 0.12.2 patch release? We can proceed with iterating on improvements for 0.13.

[davidhewitt]

(Also this could probably benefit from a CHANGELOG entry static that it fixes #1211 )

[kngwyu]

I guess with the removal of public fields this is technically a breaking change, so maybe let's merge this and abandon the idea of making a 0.12.2 patch release? We can proceed with iterating on improvements for 0.13.

Hmm... I'm not sure we should treat this as public API 🤔

[sebpuetz]

Changes here are unrelated?

[kngwyu]

I did for address clippy warnings.

[sebpuetz]

Although clippy only asked to replace the implementation with matches!(), no?

[kngwyu]

Yeah, but since we support MSRV(1.39), we cannot use macthes.

[kngwyu]

@sebpuetz
I updated PyCFunction to make it more backward-compatible, except now doc doesn't need to be null-terminated.

[davidhewitt]

Hmm... I'm not sure we should treat this as public API 🤔

Should we maybe be marking pyo3::class::PyMethodDef etc. with #[doc(hidden)] then? I'm more ok with treating this as a "patch fix" if we can claim that these should never have been public API 😄

[kngwyu]

I made them #[doc(hidden)], though I leave the release decision to you.

[davidhewitt]

👍 anything else you want to change in this PR before it merges? I will release 0.12.2 soon after this is merged.

[kngwyu]

No, I think it's ready