y-crdt / ydotnet

.NET bindings for yrs.
MIT License
35 stars 9 forks source link

Improve nullability #48

Closed SebastianStehle closed 10 months ago

SebastianStehle commented 11 months ago

The y-crdt library does not null that often. If something fails, you actually get an error indicating what the problem is. Unfortunately the yffi binding is returning null for errors and therefore swallows the actual reason. Very often it is mentioned in the documentation what the problem is.

We should not make the mistake. Lets say you open a transaction in your business logic. What do you do, when the result is null? You have to log this case or throw an exception or something like that and you have to research what the error might have been. It would be more user friendly to just throw the exception. Then the code blows up and is usually handled by an exception handler and logged, like for all other exceptions.

Sometimes the nullability is just wrong, e.g.

public Doc? Clone()
    {
        return ReferenceAccessor.Access(new Doc(DocChannel.Clone(Handle)));
    }

or

public Text? Text(string name)
    {
        var nameHandle = MemoryWriter.WriteUtf8String(name);
        var result = ReferenceAccessor.Access(new Text(DocChannel.Text(Handle, nameHandle)));

        MemoryWriter.Release(nameHandle);

        return result;
    }

In both cases the result can only be null in really buggy situations.

Please assign me, if you agree. Happy to provide a PR.

LSViana commented 11 months ago

Hi, @SebastianStehle!

I agree with your proposal to fix and improve the documentation here but I think it's part of a bigger problem that I, on purpose, didn't handle in the library so far: exception handling.

You probably noticed wherever something can go wrong in the library, if it happens there's no handling at all. So, I'd suggest renaming this issue to something like "Improve exception handling" and then we can include items like:

However, I don't intend to do that in a "local" way as it's suggested here or, for example, by adding a bunch of if statements throughout the code, I'd like to find a global approach to throw exceptions keeping the Rust stack trace and without crashing the process, for example.

So, if you agree, let's try to make this scope bigger and investigate how to handle this.

LSViana commented 11 months ago

Also, please keep in mind that the priority of this kind of task is low for now because I'm still improving the features and memory management.

In the best case, for now, if there are no programming errors, the exceptions shouldn't happen very often.

LSViana commented 10 months ago

Hi, @SebastianStehle!

Since there are many issues in a row that could be closed after #55 (I think this is the 4th or 5th already), I'll just go ahead and close them for now.

If you notice there's anything else pending to do, please feel free to re-open any of them or just open new issues so we can start discussions with a clean slate.