Create polished version of generate_reflection named rbx_reflector

[nezuo]

This is a continuation of the #221 PR. I thought I would start a draft so I could get feedback.

[nezuo]

I've added the DefaultsPlace command but there are still a few things I would like to fix:

1. The old generate_reflection used innerput to save the place automatically but that wasn't working for me. I think this will be necessary for a CI workflow.

2. Since it calls kill on the studio process studio_process.kill()?;, it leaves behind a lock file. I could delete the file but I was wondering if there is a way to stop the process more gracefully.

[nezuo]

Is there a rule on comment wrapping?

[nezuo]

I added the Generate which for now only takes an API dump and outputs a serialized ReflectionDatabase. I changed the API of the Dump command to output the API dump instead of the reflection database. I'm not sure what the API was supposed to look like so I'll focus on getting everything functional.

[nezuo]

I'm not using the latest version of serde_yaml to deserialize the patch files because 0.9 changes the way enums are handled:
https://github.com/dtolnay/serde-yaml/releases/tag/0.9.0

[nezuo]

I'm going to open this up for review because I would like some feedback on a few things.

1. innerput needs to be fixed.
2. rbx_xml and rbx_binary need a reflection database for their tests. Should I just generate a serialized reflection database to use for the tests? rbx_util also needs a reflection database.
3. What should I do about the lock file?
4. Should I use the env_logger crate?
5. Should it be up to the user to delete files they don't need anymore like the api dump or the defaults place?
6. What was the Patch command supposed to do?

[kennethloeffler]

innerput needs to be fixed.

Elaborate on how it's not working? You could be running into one of innerput's failure cases. It fails when:

• An application using the Windows Console terminal emulator is focused (i.e. cmd.exe, powershell.exe [note that in recent releases of Windows, these use the newer Windows Terminal, which does not have this problem])
• The start menu is focused
• Some other Fun Windows Things that I don't remember at the moment (I really should have written them down)...

Since I wrote it, it has worked reliably for me as long as I avoid these cases, so either I messed up in the unsafe, or one of these things is happening. These problems must be fixed when we expose rbx_reflector to end users (can't wait 😔), but I think it can work for CI. Please leave as much information as possible and feel free to get in touch with me over Discord or email if you want to have a chat about it - it's important that this works as expected

rbx_xml and rbx_binary need a reflection database for their tests. Should I just generate a serialized reflection database to use for the tests? rbx_util also needs a reflection database.

I think rbx_xml, rbx_binary, rbx_util can stay as-is and we'll just use rbx_reflector to generate the static databases in rbx_dom_lua and rbx_reflection_database, same as we've been doing with generate_reflection. I think changing everything over to support dynamic databases is out of scope for this PR and it would be better to tackle in a future PR

What should I do about the lock file?

I'm not totally sure on best practice for Cargo.lock in workspaces that include both libraries and binaries either. We can take a look at other large projects that need to think about this (ripgrep for example, which provides a binary as well as libraries, sets a binary target in Cargo.toml, and checks Cargo.lock into vc).

Should I use the env_logger crate?

I think we should, the configurable logs in generate_reflection have come in handy many times for me

Should it be up to the user to delete files they don't need anymore like the api dump or the defaults place?

generate_reflection puts these in a temp directory that gets deleted when we're done with it - I think we can do the same here

What was the Patch command supposed to do?

I'm not sure! Maybe it was supposed to apply patches to a given database in isolation? I can see this being useful, sometimes you want to separately run and inspect the results of each step of the generation process

[nezuo]

Elaborate on how it's not working? You could be running into one of innerput's failure cases. It fails when:

It's failing to find the window for the studio process.
https://github.com/kennethloeffler/innerput/blob/5d81e78cc4a666ba31f57662b266e4655cb8a6a5/src/win32/window.rs#L210

I should have been more specific about the lock file. I was referencing my earlier comment here:

Since it calls kill on the studio process studio_process.kill()?;, it leaves behind a lock file. I could delete the file but I was wondering if there is a way to stop the process more gracefully.

generate_reflection puts these in a temp directory that gets deleted when we're done with it - I think we can do the same here

rbx_reflector is split up into multiple commands, if I want to generate a reflection database, I have to pass in file paths for the api dump, patches, etc. That's hard to do if they are put in a temp directory and it makes it hard for the user to inspect the result of each command. Would it be reasonable to expect the user to just add these files to their .gitignore?

[Dekkonot]

I tried this (hence the existence of this commit), but it seems like the defaults place crashes Roblox Studio on close (possibly due to a specific instance type? dunno 😅)

@kennethloeffler It feels like maybe we should track down what's causing that issue because it'd make for an interesting bug report but... That's irrelevant to this PR, I guess. Shame it causes a crash. :(

@nezuo As per the discussion on Discord, I think we should just focus on generating a database cleanly. The patch command (whatever it was meant to be) can come later since right now it's not clear what functionality we actually need. Let's not get caught up in design and instead just make something that's a production ready version of generate_reflection.

[Dekkonot]

That CI failure isn't your fault, don't worry. Since this replaces generate_reflection, we should probably remove it once this gets merged which would fix this issue. That said, I'm just going to remove the pinned build CI (#270) because it doesn't make sense for us to do that beyond clippy maybe failing. It doesn't fix it right this minute but it will eventually.

Because this fundamentally changes how we write patch files, would it be possible for you to update docs/patching-database.md? It's a bit involved because of the nature of this change but maintaining this guide is important.

[Dekkonot]

I cannot believe I almost forgot this but we need a README for this tool. Just a simple description of what it does and how to use it will do since it's internal. If you need an idea, you can look at the README for generate_reflection.