japhib / pico8-ls

PICO-8 Language Server
MIT License
64 stars 8 forks source link

Usages/definitions don't work across included files on Windows #22

Closed electricbrass closed 1 year ago

electricbrass commented 1 year ago

I am having different behavior on two different computers with no changes made to settings. I don't know which is the intended behavior. On one, every one of my files is saying it has undefined functions (because they were defined in other files). On my other computer, there is not a single warning, and it recognizes them as the same functions and variables across files.

japhib commented 1 year ago

I need quite a bit more information in order to reproduce this issue. What operating systems are these 2 computers? Are the other files included using “#include” statements? Do you get any output in VSCode’s Output tab?

electricbrass commented 1 year ago

Yea sorry I figured you'd need more information but I wasn't sure what. The one where the problems do show up is Windows 10, the one where they don't is Ubuntu MATE 22.04.

Output on both has PICO-8 Language Server starting. PICO-8 Language Server launched.

There is one .p8 file with #include statements, no other files have them as Pico-8 doesn't support that.

On the linux machine, trying to call a function that doesn't exist doesn't give any warnings or anything. So it seems like its working as intended on windows? Though it would be nice to have an option for it to not need includes, and recognize things across files regardless, so that files can be used more like pico-8 tabs.

Both are VSCode 1.70.0

japhib commented 1 year ago

In my testing, #include statements are working fine across Windows, MacOS, and Linux, and functions that are defined in another file but not included correctly have a warning.

Can you provide a minimal reproducible example? e.g. like 2 small files that have nothing else in them except what is necessary to reproduce the problem. Screencasts are good too.

Though it would be nice to have an option for it to not need includes, and recognize things across files regardless, so that files can be used more like pico-8 tabs.

That wouldn't work with pico-8 though, since different tabs in pico-8 all go into the same .p8 file. We already support working with multiple files in a way that is supported by pico-8, which is the #include statements.

electricbrass commented 1 year ago

I tried what you said and made a small example. lsdemo.zip

I deleted the #include statement, and on the Windows computer it said foo was an undefined variable. On the Linux machine it did not could still recognize it. Strangely I came back an hour later and it worked properly on Linux. But only in this example. Not in my actual project.

electricbrass commented 1 year ago

That wouldn't work with pico-8 though, since different tabs in pico-8 all go into the same .p8 file. We already support working with multiple files in a way that is supported by pico-8, which is the #include statements.

Thing is that #include statements can only go in the main .p8 file. And because of the limitations of editing that file while also having Pico-8 open, it can be desirable to have it be just some import statements and all code in other files. This means I can use each file similar to a tab in Pico-8, but I get warnings because of things not being included in each file.

japhib commented 1 year ago

Wouldn't you have to edit the file every time to get it to work in PICO-8, though? Like, PICO-8 isn't going to just recognize variables in other files just because you want it to.

Plus, there's a big downside to having this language extension just automatically treat everything in the folder as if it's the same project, and that is naming clashes. If you have multiple .p8 files open and they both have common things like entity, player, etc. it'll show up as if it's defined in other files, but then you'll go run your actual cartridge in PICO-8 and it'll error because those other files aren't included.

electricbrass commented 1 year ago

This is what's in my p8 file. It works just fine. image

And I'm suggesting it to be a toggleable option that is not on by default.

japhib commented 1 year ago

What do you mean "it works just fine"? What do you want to toggle?

japhib commented 1 year ago

Btw, I ran your example on Linux and I get a warning undefined variable: foo when I comment out the include statement. It's not your exact version of Ubuntu, that's the only thing I can think of.

So anyways, unless you'll be able to provide more help in reproducing this problem, I won't be able to look into it. However, I'm happy to answer questions and offer guidance if you want to try cloning this repo and debugging the issue yourself.

electricbrass commented 1 year ago

Wouldn't you have to edit the file every time to get it to work in PICO-8, though? Like, PICO-8 isn't going to just recognize variables in other files just because you want it to.

The p8 file is just these includes. The code in it is not being edited unless I add a new file. I started with tabs and made each tab into it's own file for ease of editing and organization in vscode. I thought you were implying that somehow that would not work. But it does work totally fine.

Plus, there's a big downside to having this language extension just automatically treat everything in the folder as if it's the same project, and that is naming clashes. If you have multiple .p8 files open and they both have common things like entity, player, etc. it'll show up as if it's defined in other files, but then you'll go run your actual cartridge in PICO-8 and it'll error because those other files aren't included.

I'm suggesting that recognizing the files in the folder as part of the same project is something that could perhaps be toggled.

electricbrass commented 1 year ago

Btw, I ran your example on Linux and I get a warning undefined variable: foo when I comment out the include statement. It's not your exact version of Ubuntu, that's the only thing I can think of.

Yea this example started working for me a while after making it.

So anyways, unless you'll be able to provide more help in reproducing this problem, I won't be able to look into it. However, I'm happy to answer questions and offer guidance if you want to try cloning this repo and debugging the issue yourself.

I can make my repo public so that you can see what happens if you clone it and try opening it in vscode. I get it if you don't want to.

japhib commented 1 year ago

It should already be supported. When you have a p8 file that includes those other lua files, it should recognize it as all one big project.

Sure, if you want to make your repo public, I can try opening it and seeing what happens. You'll have to give more specific instructions on which lines in which files you comment out, and where you expect to see the warnings but don't.

electricbrass commented 1 year ago

Here is the repo: https://github.com/electricbrass/Breakout-Pico-8

As it currently is, there are zero warnings on my Linux computer. On Windows, for example in line 33 of ball.lua, there is a warning that lose_life() is undefined. (It was defined in game_loop.lua). On Linux, if I try go to definition on that line, it takes me to the definition in game_loop.lua.

I can get rid of that warning on Windows with #include game_loop.lua in ball.lua. But of course that doesn't work in Pico-8 as it doesn't support includes in other files.

electricbrass commented 1 year ago

It looks like if I delete all the #includes in breakout.p8 on linux, I get the same warnings I get on Windows with the #includes. This applies to functions I never defined. If somewhere in my code I type bar(), it tells me its undefined on Windows. On Linux, unless I delete those includes, (which definitely do not have anything called bar in them) it doesn't recognize it as undefined.

japhib commented 1 year ago

Gotcha. Sounds like the real problem is that the #includes aren't working properly on Windows.

electricbrass commented 1 year ago

Ah okay, as I originally said, I wasn't sure which was intended. I made some assumptions about how includes worked and figured it was more likely the problem was on linux.

There is stlll the lack of warnings for things that really are undefined on Linux

Oops didn't mean to close

japhib commented 1 year ago

There is stlll the lack of warnings for things that really are undefined on Linux

Can you provide an example of this happening? In my testing, undefined symbols get warnings.

japhib commented 1 year ago

Just released 0.4.7 with a fix for this. If there's another problem where undefined things on Linux are not getting warnings properly, open a separate issue for that.

electricbrass commented 1 year ago

There is stlll the lack of warnings for things that really are undefined on Linux

Can you provide an example of this happening? In my testing, undefined symbols get warnings.

I'll try 0.4.7 and see if I still have any more problems.

electricbrass commented 1 year ago

No issues on Windows for me now. Works great. And all my previous comments about adding a feature can be disregarded because they were based on an assumption that windows was working as intended.