Skip to content

Add #nullable annotations to Lua libs#4689

Open
kalimag wants to merge 10 commits intoTASEmulators:masterfrom
kalimag:pr/lua-lib-nrt
Open

Add #nullable annotations to Lua libs#4689
kalimag wants to merge 10 commits intoTASEmulators:masterfrom
kalimag:pr/lua-lib-nrt

Conversation

@kalimag
Copy link
Copy Markdown
Contributor

@kalimag kalimag commented Apr 17, 2026

Add #nullable annotations to Lua libraries for #3755 and #4688

TODO:

  • CommLuaLibrary (this one is a mess)
  • FormsLuaLibrary
  • GuiLuaLibrary
  • MovieLuaLibrary
  • TAStudioLuaLibrary
  • Add #nullable to Lua libs that currently don't have any reference types in their API surface?

Check if completed:

@SuuperW
Copy link
Copy Markdown
Contributor

SuuperW commented Apr 17, 2026

Most of the things that you added required for are not actually set via object initializers, since most Lua libraries are constructed through reflection.
I'm not sure if this is really a problem with using required. (It's a problem with reflection, but that isn't what this PR is about.)

@kalimag
Copy link
Copy Markdown
Contributor Author

kalimag commented Apr 17, 2026

Most of the things that you added required for are not actually set via object initializers

Yeah, ultimately it's just about picking a way to shut up the analyzer. IMO required is the least bad option here, at least it expresses intent even if the requirement isn't actually compiler-enforced.

@kalimag
Copy link
Copy Markdown
Contributor Author

kalimag commented Apr 18, 2026

There is an annoying mismatch between C# and Lua semantics here:

In C# (and more importantly, BizHawk's Lua bindings), Foo(string? bar) and Foo(string? bar = null) are different. But in Lua, foo(nil) and foo() are normally the same thing, and there is no way to express the difference in LuaCATS annotations.

This is an issue in cases where the last non-optional parameter in a function is nullable, e.g. client.addcheat. client.addcheat(nil) works (but is a no-op), while client.addcheat() throws an error, but LLS will warn on neither of them.

I think I'll remove/omit the ? on such parameters if it doesn't make sense to pass nil, even if the method accepts nil without throwing an error.

@YoshiRulz
Copy link
Copy Markdown
Member

If client.addcheat(nil) is a no-op, client.addcheat() should be as well i.e. the parameter should have a default value of null. (IMO both should throw.)

Copy link
Copy Markdown
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of changes to things other than nullability annotations in here...
We don't use is { } nonNullObj.
You can't make any of those member props required because all the subclasses of LuaLibraryBase are instantiated by reflection.

@kalimag
Copy link
Copy Markdown
Contributor Author

kalimag commented Apr 20, 2026

You can't make any of those member props required because all the subclasses of LuaLibraryBase are instantiated by reflection.

What's your preferred alternative?

@SuuperW
Copy link
Copy Markdown
Contributor

SuuperW commented Apr 20, 2026

You can't make any of those member props required because all the subclasses of LuaLibraryBase are instantiated by reflection.

Not all. Only the ones in Common (which is most).
I think it's fine, though. The thing to fix is getting rid of the reflection use, but that's outside the scope of this PR. At least required will be correct as soon as someone tries to construct any of the classes in the normal non-reflection way.

kalimag added a commit that referenced this pull request Apr 23, 2026
Based on in-progress #3755 and #4689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants