Add support for generating LuaCATS definitions for the Lua API#3755
Add support for generating LuaCATS definitions for the Lua API#3755kalimag wants to merge 31 commits intoTASEmulators:masterfrom
Conversation
bd55b04 to
72fc938
Compare
|
Let's move forward with this one! I decided to move from notepad++ to vscode for lua coding after 15 years, and ended up making exactly the same thing independently yesterday, I didn't even remember this PR! And here it's done in a much nicer way, so let's have it! |
YoshiRulz
left a comment
There was a problem hiding this comment.
After fixing malformed examples, works w/ SumnekoLua plugin in IDEA.
48bca5b to
246190f
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
18425a1 to
0678148
Compare
| { | ||
| // technically any string parameter can be passed a number, but let's just focus on the ones where it's commonly used | ||
| // like `gui.text` and `forms.settext` instead of polluting the entire API surface | ||
| if (parameter.Name is "message" or "caption" && parameter.ParameterType == typeof(string)) |
There was a problem hiding this comment.
We don't want to encourage this. There are enough footguns in calling gui.* already with Lua not having named arguments.
There was a problem hiding this comment.
Removing this will likely cause a lot of unnecessary warnings
There was a problem hiding this comment.
I think type coercion warnings are good, but then I also consider dynamic languages like Lua to always be inferior to statically-typed languages. Is passing numbers for strings idiomatic in general, or only in concatenation?
There was a problem hiding this comment.
Well, number-to-string conversion is specifically a feature in BizHawk's Lua bindings, not a feature of the Lua runtime. That's why LLS warns about it unless the parameter definition allows numbers.
But reading numbers from memory and passing them to gui functions is a pretty common pattern in BizHawk scripts, so I added this special case for those functions to avoid warnings in existing, working scripts. I have a test directory with the bundled Lua scripts and some from the Lua scripts catalog, and removing this code causes 73 additional warnings, 16 of which are in BizHawk's own scripts (for example).
I have no strong opinions on deprecating this behavior, but currently it's valid so the definitions say it's valid.
|
|
||
| if (parameter.Member.DeclaringType == typeof(GuiLuaLibrary) && parameter.Name == "surfaceName" && parameter.ParameterType == typeof(string)) | ||
| { | ||
| return "surface"; |
There was a problem hiding this comment.
We should introduce new attributes if this grows to any more enums.
| throw new NotSupportedException($"Unknown type {type.FullName} used in API. Generator must be updated to handle this."); | ||
| } | ||
|
|
||
| private static bool IsNullable(Type type) => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>); |
There was a problem hiding this comment.
Reference types (i.e. string and byte[]) are nullable too
There was a problem hiding this comment.
True, but this is about documenting when null/nil is valid, and in the Lua API it's generally not unless the parameter is optional.
If the Lua libraries used #nullabe annotations that information should be taken into account here, but currently that's not the case (and also I don't think .NET Framework has a reflection API to read that information).
There was a problem hiding this comment.
All reference types in the Lua API are nullable because Lua is not a .NET language and cannot "see" the NRT metadata.
There was a problem hiding this comment.
I do not follow. Are you saying want every string parameter to be marked string? in the definitions?
There was a problem hiding this comment.
Passing nil for a parameter annotated as string results in a warning from the language server, so yes they all need to be string?. Unless you want to create another attribute and mark all the functions which throw on null arguments.
There was a problem hiding this comment.
That's the point, though? gui.addmessage(nil) throws an error, so the diagnostics should warn about it. event.onexit(function() end, nil) doesn't throw an error, so diagnostics shouldn't warn.
That both are a nullable reference type on the technical level is irrelevant. As far as Lua is concerned, anything goes and you can pass a dozen table arguments to memory.writebyte if you want, but the diagnostics should help you catch that before it causes a runtime error.
There was a problem hiding this comment.
(Didn't see the edit)
Currently the assumption for reference types is: parameters cannot be null unless they are optional. That seems to work well with the existing API surface.
If there turn out to be exceptions where this doesn't hold true, I think it would be preferable to mark nullable parameters rather than not-nullable ones, and for that I'd prefer to just reuse C# #nullable annotations rather than creating a bespoke attribute, if reasonably possible.
There was a problem hiding this comment.
Reference types are now annotated as nullable, unless they are compiled with #nullable enable (and not marked as nullable in C#).
Still need to do the legwork of adding #nullable to existing Lua libs.
There was a problem hiding this comment.
Wait they are available with reflection? I was under the impression that you needed NullabilityInfoContext from .NET Core.
I'll see about removing the need for dynamic.
There was a problem hiding this comment.
Sure, it just gets rather complicated to correctly interpret the attribute data without NullabilityInfoContext once you get into generics and nested arrays and whatever. But for this purpose it's simple enough.
|
Please rebase. |
Co-authored-by: feos <vadosnaprimer@users.noreply.github.com>
I don't know regex, replacements probably can be done in a neater way
we gotta promote this!
revert if we don't want this anymore, but with LLS's ability to take in entire folders via config this feels nicer IMO and yeah I wouldn't mind adding this output to releases under something like `_luacats_definitions` folder in `Assets/Lua`
These are globals rather than `require`'d imports
d7a355e to
aee330b
Compare
the PR #3755 adds a button to generate these automatically, similar to the existing TASVideos wiki generation


Adds the ability to generate API definitions in the LuaCATS format to the Lua Console.
LuaCATS is primarily the annotation format of Lua Language Server, which drives the most popular VS Code Lua extension and can be used in other editors with language server support.
LuaCATS is an extension/fork of the annotation format used by EmmyLua, but I'm not sure what the status of the latter is. These definitions might be compatible with EmmyLua, but the only spec of its format I could find is sparse and partially in Chinese.EmmyLua extension now uses LuaCATS annotations, too.The generated definitions could be further improved, e.g. enumerating valid values for
surfaceNameparameters, specifying the structure of certaintablevalues, and callback function signatures. But that would require extending the API metadata and I wanted to get some feedback first.Current output:
Check if completed:
will resolve #3860