Detect cycle during projectivize

[kadarakos]

Description

Attempting to detect cycles during projectivize step in get_aligned_parse. Fixes #10859.

Types of change

Small bugfix

Checklist

• I confirm that I have the right to submit this contribution under the project's MIT license.
• I ran the tests, and all new and existing tests passed.
• My changes don't require a change to the documentation, or if they do, I've added all required information.

[danieldk]

Discussed with @kadarakos : I'll make some changes, so putting it in draft temporarily.

[shadeMe]

I think we could use C-enumerations (or even C++11 enum class for additional type-safety, if Cython supports it) in place of pair[bool, bool]? That would be more readable/explicit.

[danieldk]

That would be possible from the pair[bool, bool] functions, but not for pair[int, bool] (unless we hack the semantics like before). I am not sure if I like using different types for different related functions?

[danieldk]

I was saying to @kadarakos, I wish we had sum types 😭.

[danieldk]

After discussion with @shadeMe and @kadarakos, redid it with C++ exceptions. Now we have to decide which of the TMTOWTDI's we want.

[kadarakos]

I was telling @danieldk that I think its nice to have the exception, because even though it might be an overkill here, we could use it to refactor maybe some of the clunky errors with raise from cython.

[danieldk]

I think it would be nice in some places, but it also means that we have to be more mindful about except signatures. I found out today if you call a noexcept + function (can throw a C++ exception) in a cdef nogil function, the exception is swallowed? So every place we call a noexcept + function, we have to to take care that we have an except specification. Also, the exception is not propagated as a C++ exception, but as a Python exception from that point, so you need something like except * or except -1 (depending on your return value).

There are probably places in our code base where we are calling functions that may throw (except +) in cdef nogil functions, where we do not take proper care of propagating the exception.

[danieldk]

I am still not sure if this is a net improvement over returning errors as values and maybe having a template class for errors. At least, something like

cdef Result[int, string] myfunction(...) nogil

is harder to mis than an except + that is only visible in some pxd.

[kadarakos]

hmm. What do you think of having always tuple[Any, bool, bool, bool, ....] where the Any is the actual value and the bools refer to various errors that can be encountered?

[danieldk]

That has very weak semantics. The benefit of the Result approach is that the error could be anything strongly-typed (like an enum class or string).

[kadarakos]

So what's left now is to add some cycle-detection test cases to is_non_proj_arc ?

[danieldk]

So what's left now is to add some cycle-detection test cases to is_non_proj_arc ?

Yeah and is_non_proj_tree.