Fix type resolution for cross-directory thrift includes

[FrankPortman]

Summary

The thriftpy2 bump to >=0.5.0 was trickier than I thought. Not only was there the whole _thrift appending inside the module map, but also the behavior changed in terms of how cross-module imports were handled.

I switched to building a __thrift_module_name__ -> __name__ mapping from includes and threading it through type resolution. This has the side effect of also fixing the _thrift suffix stuff.

Also includes new parity/import tests across all flag combinations.

Discussion

thrift-pyi already flattens all output into a single directory with a single __init__.pyi that re-exports every module as a sibling.

So when we encounter a cross-directory include like include "some/nested/path/Foo.thrift", the natural thing to do is resolve it to its leaf module name Foo, because that's what the generated Foo.pyi will actually be called in the output directory. This is also how Thrift itself works when you include files (you ref them in the importing file via their base name only).

For same-directory includes, the mapping entries are effectively identity (Foo_thrift -> Foo), which is the same thing removesuffix("_thrift") was already doing.

This is fully backwards compatible, but this regression caused me to wonder if you had actually intended to flatten everything out, or if you only ever ran this from within one directory/child at a time. That said, this is one reasonable interpretation of how cross-dir includes should work.

If there's a case where preserving some of the directory structure in type annotations would be preferable, happy to discuss alternatives (CC @unmade). In my monorepo case, the flattening actually ended up being quite nice, but of course there is an obvious edge case in terms of file basenames colliding (I have a custom hook in my monorepo to avoid that, since I run it via bazel).

I think one obvious followup, is to do some sort of mangling of imported files, so conflicts CAN exist, provided they are valid thrift (and so one file doesn't import 2 conflicting files). But I need to double check whether we can rely on thriftpy2 handling this edge case correctly for us - including any scenarios where it should error instead of parse - so that we can build on it.

Disclaimer - any time I have to touch thriftpy2, I lean pretty heavily on LLMs, because I find the module map stuff pretty confusing. If there are any aspects of this that feel too "slop", let me know and I'm happy to clean it up. I did go in and personally tweak, clean, etc, all generated code but I may have missed a spot.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d751a06) to head (9f7048d).

Additional details and impacted files

@@            Coverage Diff            @@
##            master       #66   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           87        80    -7     
  Branches         1         1           
=========================================
- Hits            87        80    -7     

Flag	Coverage Δ
unittests	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[FrankPortman]

@unmade the TLDR here is that the thriftpy2 bump broke cross-dir includes in my setup. thrift-pyi flattens everything out, but worked fine in that setting before. It is somewhat related to their _thrift implicit change, but not solely. But this change covers both of those scenarios in one go (mostly undoes the last PR I wrote 😄 ).

I am not sure if you had specifically thought about the nested dir and/or cross import setting before, but imho the "flatten everything" is reasonable behavior, given how Thrift include statements work. But ofc, there are alternatives, now that I have opened pandora's box here.

[unmade]

I am not sure if you had specifically thought about the nested dir and/or cross import setting before

Never thought of the nester dir. At my company we used flat structure, so it was just due to thriftpy2 < 0.5 that everything worked.

If there's a case where preserving some of the directory structure in type annotations would be preferable

I checked and I think flattening is the right way to go as this is what thrift does itself. So I don't think we need any alternative there. The solution looks good.

I need to double check whether we can rely on thriftpy2 handling this edge case correctly for us

I checked it and thankfully thriftpy2 handles that case. For example if I add another_sub/child.thrift and import it in the parent.thrift, then I'd get:

ThriftGrammarError: 'child' type is already defined in 'includes'

If there are any aspects of this that feel too "slop"

No slop spotted, thanks for the work! 💪

[unmade]

nit: I'd rather introduce some nested folders as part of existing examples/interfaces folder.

If that's OK with you, I will implement that change. For now I'm thinking of splitting shared.thrift into multiple files under different folder.

[unmade]

On the other hand I like that we have isolation for the cases, so I might just move cross_dir_interfaces under example folder.

[FrankPortman]

I defer entirely to you on this. I did feel awkward the way I set it up but I was trying to avoid having this PR shake up too many details about other tests.

[unmade]

I added nesting directly to example/interfaces, so now it has common/labels.thrift which is imported in the top-level todo.thrift.

By a chance I noticed that in some cases value wasn't not prefixed with a module name, specifically in that case:

9: required list<labels.Label> labels = [labels.DEFAULT_LABEL]

In the stub file it was produced as follows:

labels: List[labels.Label] = field(
        default_factory=lambda: [Label(name="default", color=3)]  # `[Label(...)]` is missing `labels` module
    )

I had to rethink how we add module name to the value and I couldn't find anything better than to monkey patch __repr__ method to include module name. Not super happy monkey patching, but it works and it is pretty straightforward.

[FrankPortman]

Hmm I will have to digest this tomorrow. As far as I remember we had fixed that case a while ago (but I do vaguely remember that being an issue before). Maybe I am misunderstanding the new edge case you found, but I'll look at the commits tomorrow. Don't hold on me re landing though if you feel good about where it ended up.

[unmade]

Ah, apologies if I caused any confusion 😅. Resolving module names is convoluted indeed.

As far as I remember we had fixed that case a while ago (but I do vaguely remember that being an issue before)

I do remember that as well and briefly checking I think it was done in #61. It is very similar to the case I'm talking about here. The only difference is that #61 fixed it for simple values, say:

7: required dates.DateTime createdWithDefault = dates.EPOCH

createdWithDefault: dates.DateTime = field(
        default_factory=lambda: dates.DateTime(  # `DateTime` prefixed with `dates`
            year=1970, month=1, day=1, hour=0, minute=0, second=0, microsecond=0
        )
    )

But we didn't cover the case when DateTime was part of collection (e.g. createdWithDefault = [dates.EPOCH]). I guess we never needed it, but for the sake of correctness it is now covered as well.