refactor: Fix a bug in reorganize_definitions for uses of foreign types

[ahomescu]

• Fixes reorganize_definitions produces broken code on libexpat #1733.

[randomPoison]

Can you give a bit of context for what this change is accomplishing and why it's needed, e.g. what bug you're trying to fix? I'm not intimately familiar with reorganize definitions, nor with the CRISP changes that I assume are the motivation for this, and I'm having trouble understanding from the test change what the expected change in behavior is.

In general, it'd be helpful to provide this context in the PR description. Reviewing a change without context is hard and time consuming because I have to do a bunch of digging to figure out the context for myself.

[ahomescu]

Can you give a bit of context for what this change is accomplishing and why it's needed, e.g. what bug you're trying to fix?

We don't seem to have a bug yet, I can go open one. Stuart reported a crash in reorganize_definitions when transpiling libexpat.

[ahomescu]

I'm not intimately familiar with reorganize definitions

I'm not sure who else is, other than me.

[randomPoison]

We don't seem to have a bug yet, I can go open one.

That's fine, I don't necessarily need a ticket for it, but it'd be helpful if you can explain here what behavior you're trying to change.

I'm not intimately familiar with reorganize definitions

I'm not sure who else is, other than me.

That's also fine, I'm still able to review this, and reviewing changes like this is part of building an understanding of the refactorer and this transform. But since I (and others) don't have the context already, it's helpful if you can provide that context.

[kkysen]

Could you put the new test code in a new snapshot test? The current one doesn't compile after reorganize_definitions, so it's missing an important part of testing (which would catch this bug). So could you put it in its own snapshot file, initially set .new_expect_compile_error(true), and then remove that in the 3rd commit that fixes the bug?

Otherwise, the changes to the refactorer and to the snapshot LGTM.

[ahomescu]

So could you put it in its own snapshot file

That was my original approach, but it seemed like we already put all the tests for a transform in one file? Or should I use named?

[kkysen]

LGTM now besides sorting the tests.