Support for analyzing multiple local modules (multi-module projects) by using PYTHONPATH #176
Conversation
khatchad
commented
Mar 26, 2024
•
khatchad
commented
- No notion of PYTHONPATH in analysis #163
- Importing a module on the PYTHONPATH doesn't work #177
- Importing a module for a package on a PYTHONPATH doesn't work #178
- Support for relative imports (intra-package references).
* First test. * Add comment. * Test 2. Should fail. * More tests. * Add comment. * Extract local variable refactoring. * Improve variable name. * Add logging. * Decrease logging level. * Fix bug with determining whether a module is "local." The old code didn't consider the full path of the module, which is what is contained in `localModules`. But, it still doesn't do what I think it should do, which makes me believe that the generated CAst is incorrect. * Replace slash with dot in test filenames. * New test. This one has a sibling script in the module that uses the other sibling. * Implement `getSignature()` for `com.ibm.wala.cast.python.parser.PythonParser.translateToCAst().PythonScriptEntity<T>`. No signature currently so return `null`. * Actually add the test files. * Fix filename for test. * Remove redundant log. * Fix compilation error. * Only test PYTHONPATH for Jython3. It's only supported for Jython3. * Add docs.
|
This is only supported by Jython 3, but http://github.com/wala/ML is still working with Jython 2 due to #42. Thus, I"m unsure whether this should be merged here. |
|
We can merge it, but the new functionality isn't being tested here until #42 is fixed and both |
|
@khatchad how critically do you need this merged? I probably won't have time to do a real review for a couple of weeks, but I can stamp it if you need it in quickly |
|
We've been using our fork, so no rush. |
* More test files. * Fix test file. * Add classes. * Add test. * Add test. * Add test. * Add test. * New test. * New test. * Fix test code. * New test. * New test. * New test.
Pull request was converted to draft
khatchad
changed the title
* Remove periods from end of logging statements. Period is a valid path now with relative imports. * Fix the resolved relative import path. * Add test directory to PYTHONPATH. * Fix a bug with the wrong instance key. We need the instance key for the node that we are examining in the loop not one that we are creating constraints for.
|
@khatchad with 236 modified files this is rather intimidating to review. Will it be easier once some other PRs land? |
Unfortunately, this is quite a pervasive change. We're basically adding something like a |
| } | ||
| } | ||
|
|
||
| String yuck = moduleName; |
There was a problem hiding this comment.
That was in the original code :). My guess is that @juliandolby was frustrated with the requirement of lambdas to only accepted (effectively) final variables. See this unchanged code.
| for (File pathEntry : pythonPath) { | ||
| Path modulePath = | ||
| localModule | ||
| .map(SourceModule::getURL) | ||
| .map(URL::getFile) | ||
| .map(Path::of) | ||
| .orElseThrow(IllegalStateException::new); | ||
| LOGGER.finer("Found module path: " + modulePath); | ||
|
|
||
| if (modulePath.startsWith(pathEntry.toPath())) { | ||
| // Found it. | ||
| Path scriptRelativePath = pathEntry.toPath().relativize(modulePath); | ||
| LOGGER.finer("Relativized path is: " + scriptRelativePath); | ||
|
|
||
| // Remove the file extension if it exists. | ||
| moduleName = scriptRelativePath.toString().replaceFirst("\\.py$", ""); | ||
| LOGGER.fine("Using module name: " + moduleName); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This code seems a bit duplicated with code earlier in the PR? Is it worth extracting a method?
There was a problem hiding this comment.
Good catch, thanks. I'll have a look.
There was a problem hiding this comment.
It's a little difficult to fully extract this since it's in a loop with a break. I think i can extract some of it at least.
There was a problem hiding this comment.
I gave it a shot.
