justarandomgeek / vscode-factoriomod-debug

Factorio Mod Tool Kit
Other
100 stars 24 forks source link

Fix the type of global not carrying across files #105

Closed JanSharp closed 10 months ago

JanSharp commented 10 months ago

This fixes

---@class MyGlobal
global = {}

not affecting the type of global in any file outside of the one with this annotation. This seems to be caused by the language server having some understanding of lifetimes of globals, as doing global = {} makes it lose type information in code folloing that statement, or something.

The fix works by removing the plugin injected assignment to global at the top of each file. Instead it adds a diagnostic disable on each usage of the renamed global.

There are newlines before and after this diagnostic disable as to not affect any other code.

These newlines do not cause any other issues themselves, however in order to position the preceeding one properly it had to include the preceeding character before the word global in the diff. This can cause said character to get duplicataed if there is another diff that also includes this character, such as code like remote.call("foo", "bar")global.foo = "bar". Note how there is no space between ) and global. I consider this to be small enough of an issue to be worth getting undefined global warnings in cases like foo.bar = global.bar for foo.

The newline after the injected diagnostic disable does not have any side effects that I'm aware of, so this gives a warning that the global foo does not exist global.bar = foo.

Overall this does worsen the performance of the plugin, however not by much (a more complex pattern for globals, including creating strings for all preceeding text on all lines containing global, similar to the logic for remote). If it's a concern I can do measurements, however I'm not worried.

JanSharp commented 10 months ago

Wait, hold up. If it's adding a newline before and after anyway, there's no need for the block comment. I gotta fix that real quick.

justarandomgeek commented 10 months ago

image this combined with the remote.call rewrite seems to cause some trouble

remote.call("conman","set_preload_string",global.conman.unit_number,"TEST")

produces these errors, but adding a space before global fixes it:

remote.call("conman","set_preload_string", global.conman.unit_number,"TEST")

If it matters, this is in a mod scenario control.lua (as shown in the file path in the screenshot)

JanSharp commented 10 months ago

Damn it, I guess it really can't just blindly assume that it won't cause issues, because clearly it did. There is a fairly optimized way to check for an existing diff at that location, and I think it's worth adding that in order to still get undefined global warnings on the same line as (and before) global. I'll implement it and try to do some crude performance measuremnts.

JanSharp commented 10 months ago

Fixed, and it even emits the warning on the left side :D image To my surprise performace is barely impacted, but I suppose that's string operations vs table operations. The newly added logic contains very few allocations, so I guess that makes sense.