Converted `Memory` into a `readonly struct`

[martindevans]

This is a demonstration of what's involved with converting Memory into a readonly struct, as discussed in (https://github.com/bytecodealliance/wasmtime-dotnet/pull/219#pullrequestreview-1296613008). As you can see this was almost no work. The bulk of the changes are fixing the tests.

My only concern with this is that it is now possible to call things on a default(Memory), which of course would have simply triggered a null reference exception before. I've checked through and I think it's safe in this case because everything uses store.Context which will trigger a null reference exception.

Thoughts?

[martindevans]

this will cause boxing to occur where this is used, for example with Linker.Define(string module, string name, object item)

If it's just an internal interface I would change it to Linker.Define<T>(string module, string name, T item) where T : IExtern. Which would also have the benefit that you can't pass non-externs into it (which is currently checked at runtime). That won't work for the public API though since IExtern is internal. For the public API can we just add overloads for all the individual extern things, there are only 4 (Memory, Table, Global, Module) as far as I know?

[peterhuene]

For the public API can we just add overloads for all the individual extern things, there are only 4 (Memory, Table, Global, Module) as far as I know?

That sounds reasonable to me!

[martindevans]

MacOS CI failure is a bit cryptic, I'm hoping it's just another transient error.

[kpreisser]

Hi, it seems you intended to rebase the PR on current main branch (resulting in 7e2d02a8ae896e63271d8e9c89b1acfa7efcc841), but then merged it with the previous commit of this PR (617be6fd5fb7c407d6fac36b6e03ed60ff12a533), which is why the Github UI is now showing changes that are already part of main.

Can you reset the branch to commit 7e2d02a8ae896e63271d8e9c89b1acfa7efcc841 and force-push it (as I think that was the intended commit)? Thanks!

[martindevans]

Thanks for the hint @kpreisser, I didn't notice that the history was in such a messy state! Doing as you suggested has fixed it.

[martindevans]

Unfortunately I don't have access to a Mac to investigate the test issues. There's no detail in the log on what the problem might be either :/

[kpreisser]

I'm also not sure why it's crashing, but maybe it is caused by/related to the MemoryAccessTests that allocate a 4 GB Memory (ItCanAccessMemoryWith65536Pages + ItThrowsForOutOfBoundsAccess). Can you try to add the same attribue as for the Memory64AccessTests to skip them, to check if that avoids the crash? Though I'm not sure why this would happen on this PR only.

https://github.com/bytecodealliance/wasmtime-dotnet/blob/9562c78652208c49e55c6618d4ff64103dfec9e9/tests/Memory64AccessTests.cs#L28

Thanks!

[martindevans]

After skipping some tests and commenting them back in one by one I've narrowed it down to this test:

[Fact]
public void AccessDefaultThrows()
{
    var memory = default(Memory);

    Assert.Throws<NullReferenceException>(() => memory.GetLength());
}

I added this test to reproduce the problem:

[Fact]
public void TestMacOsCI()
{
    try
    {
        object? o = null;
        o.Equals(o).Should().BeTrue();
    }
    catch (NullReferenceException)
    {
        return;
    }

    Assert.False(true);
}

This crashes the test runner!

[martindevans]

Superseded by #235, which allows Memory to remain as a class.