search

RyanLamansky / dotnet-webassembly

Create, read, modify, write and execute WebAssembly (WASM) files from .NET-based applications.
Apache License 2.0
792 stars 73 forks source link

Error improvements, part 1 #52

Closed munik closed 2 years ago

munik commented 2 years ago

@RyanLamansky Please let me know if these changes are more in line with what you're hoping from the development objective: "Improve exceptions thrown from problems found during compilation or execution." Do you think it would be worth going ahead and factoring out the remaining sections from Compile.FromBinary? I'm happy with whatever you decide -- part of me thinks it would be better to be consistent, but I don't feel strongly.

~~Note: My changes in ab473ee8330ee54fece20ec6d797218f9e37a1bc introduced test failures in SpecTest_i32 and SpecTest_i64 because rotr wasn't defined on the IntegerMath class, so I defined it as it looks like all the other methods from the JSON files were expected to be defined on that class. I'm not entirely sure what's going on with those tests -- an explanation would be helpful so I understand.~~

Also, just FYI I've started working on an F# wrapper around your library as the ultimate project I'd like to use your library for will involve consuming it from F#: https://github.com/munik/dotnet-webassembly-fsharp. Let me know if you have any input!

munik commented 2 years ago

Actually, on second thought, I'm wondering if AssertExportedMethodExists should check for methods all the way up the inheritance hierarchy rather than only on the derived class. Now that I think about it, the errors I got from rotr and the early return I added for typeof(object) seem to indicate that exportsBuilder.DefineMethod allowed inherited methods

munik commented 2 years ago

After looking into it some more, that last commit is not ready. I didn't realize that the TypeLoadException is actually being thrown from the var exportInfo = exportsBuilder.CreateTypeInfo(); line. Additionally, I think it would be better if we don't check before the exception is thrown -- both to avoid overhead and to not have a false positive if the check were ever incorrect in some small way. I still think it would be nice to have some diagnostics for this, as the default message thrown is not very helpful, but I need to put more thought/work into it.

munik commented 2 years ago

I went ahead and removed the last commit from the PR

munik commented 2 years ago

I pushed fixes for your feedback. Thank you for the review!

RyanLamansky commented 2 years ago

Merged!