fix: python imports

[nohehf]

Hello @hendrikvanantwerpen, this fixes #430. This issue was that a symbol was poped for each part of the file path, but it was pushed (on import) only for each part of the import. (eg. it pushed Users, nohehf, module, foo and popped module, foo). Languages like python needs knowledge about the "root path", as modules are resolved relative to the "input script" as stated here: https://docs.python.org/3/tutorial/modules.html#the-module-search-path

Here is my fix proposal, and I believe it could be useful for several other languages:

• introduces a ROOT_PATH global variable, that allows to compute the relative file path. This is particularly useful for interpreted languages like python that creates the modules paths relative to a base path.
• adds a test that reproduces the error as described in #430, it did fail before the changes and now passes
• FILE_PATH is now passed explicitly, rather than extracted from stack_graph[self.file] as it's stated in https://github.com/github/stack-graphs/blob/558d1da3c27dfe50a45cc9acbee66bc5d898d314/stack-graphs/src/graph.rs#L335-L337 that it is not necessarily the file path.

Let me know what you think.

[hendrikvanantwerpen]

I think in general this is the right idea. I'll look into the details later today.

[nohehf]

Thanks for the review, will look into it and make those changes by the end of the weekend I believe. I agree moving it in the globals is overall a good approach for the reasons you mentioned, however don't you think that this could lead to some runtime errors as it will not be checked anymore ? I think at least file path (but probably root path too) are standard and should be required somewhere (/ checked at compile time) ?

[nohehf]

I implemented the changes, indeed it's a far better cascade. I also thought that we might want to create variables directly from the file path, as it is always needed Variables::from_file_path(&Path) or something like this (it would reduce a bit of the code duplication but not sure it's needed). Also, this approach makes it that FILE_PATH could be missing, and that will crash at runtime (but tests should ensure this I believe)

[hendrikvanantwerpen]

don't you think that this could lead to some runtime errors as it will not be checked anymore

It is true that callers can now forget this, which would lead to a runtime error. I think the impact of this problem will be low, since any attempt to run the TSG without the right variables set will fail. (It's not silently set to a default unless one is explicitly set in the TSG.) So any basic test triggering this code path should immediately reveal the problem.

The idea we have for long term is that callers wouldn't have to set these variables explicitly. Instead, languages would have some kind of source folder analyzer as part of their configuration. That would determine which files in a directory can be analyzed as part of the languages and which variables are set for them.

That being said, with these changes we are in an awkward middle. We might keep the FILE_PATH fallback in the library, so that when it's not set already, we set it to the file path associated with the file.

[nohehf]

Thanks for the review, I added the requested changes

[nohehf]

Okay, except the conditions are inverted. I'll commit those suggestions myself and start a CI run to see how we are doing.

Oh sorry my bad on this one, did this late and did not pay attention, sorry

[hendrikvanantwerpen]

🎉 Thanks for the contribution @nohehf!