[Python] The Pants Rust inference parser exhibits some dependency misinterpretation issues when python-infer.string_imports is enabled.

[liudonggalaxy]

Describe the bug The Pants Rust inference parser exhibits some dependency misinterpretation issues when python-infer.string_imports is enabled. This is evident in the following examples:

1. The Rust parser in Pants could not infer dependencies across multiple lines correctly. In the following example, the Rust parser identifies a.b as a dependency, whereas the classic parser correctly discerns a.b.c.d:

   @mock.patch(
   'a.b.' 
   'c.d', 
   new=mock.PropertyMock(return_value=0.7))

2. For the string BASE_PATH = 'a.b.', the Rust parser mistakenly treats it as a dependency, unlike the classic parser.
3. In print('retrying...'), the Rust parser incorrectly interprets it as a dependency, while the classic parser does not.

To compare the Rust and classic parsers, the following commands were executed on both MacOS and Linux:

mkdir -p pants_inference
pants --python-infer-no-use-rust-parser peek :: > pants_inference/classic_parser.json
pants --python-infer-use-rust-parser peek :: > pants_inference/rust_parser.json
diff pants_inference/classic_parser.json pants_inference/rust_parser.json

Here is the pants configuration in pants.toml.

[python-infer]
unowned_dependency_behavior = "error"
use_rust_parser = true
string_imports = true  # Infer a target's dependencies based on strings that look like dynamic dependencies

Pants version 2.17.0

OS Both MacOS and Linux.

Additional info

[thejcannon]

1 seems like a bug. We'll likely have to find the right tree-sitter incantation for that.

2 and 3 are more of a "quirk" than a bug. However, since they aren't valid dotted module names (as far as I know) I think it's safe to also "fix" this quirk.

[cognifloyd]

Similar to point 2: the rust parser also causes a warning for every file that has either of these strings, neither of which is a valid python module:

• .
• ..

I get that using pants 2.18.2 with this in pants.toml (from the StackStorm project):

[python-infer]
string_imports = true
string_imports_min_dots = 1  # tools/config_gen.py has import strings with only one dot.
unowned_dependency_behavior = "ignore"
ambiguity_resolution = "by_source_root"
use_rust_parser = true

The old parser (in 2.18.x) explicitly required alphanumeric characters before the dot: https://github.com/pantsbuild/pants/blob/3da5ae57fecd0b3dada9509a25561a1db6fe64a9/src/python/pants/backend/python/dependency_inference/scripts/general_dependency_visitor.py#L26-L31

The new rust parser, however, seems to only exclude strings with a whitespace or a \ char (assuming I'm reading this rust code correctly): https://github.com/pantsbuild/pants/blob/3632a66bc7918ace25c0aa6f05b4b6a4253d908e/src/rust/engine/dep_inference/src/python/mod.rs#L325-L328

Can we make the rust parser be more selective on which strings might be dependencies? In particular, a string that only consists of . chars, or maybe any string that ends in . should be excluded (as that would also catch points 2 and 3: 'a.b.' and 'retrying...').

Maybe with something like this:

        if !text.ends_with(".") && !text.contains(|c: char| c.is_ascii_whitespace() || c == '\\') {
            self.string_candidates
                .insert(text.to_string(), (range.start_point.row + 1) as u64);
        }

Oh. That wouldn't work because the detected strings are used for both imports and assets. So, the additional logic to exclude irrelevant strings would need to go in the python side of things: https://github.com/pantsbuild/pants/blob/8eb555760cc743aa5f85b3f8ca03009bbf5213e6/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py#L117-L130

So, maybe:

    if python_infer_subsystem.string_imports or python_infer_subsystem.assets:
        for string, line in native_result.string_candidates.items():
            slash_count = string.count("/")
            if (
                python_infer_subsystem.string_imports
                and not slash_count
                and not string.endswith(".")
                and string.count(".") >= python_infer_subsystem.string_imports_min_dots
            ):
                imports.setdefault(string, (line, True))
            if (
                python_infer_subsystem.assets
                and slash_count >= python_infer_subsystem.assets_min_slashes
            ):
                assets.add(string)

[cognifloyd]

Another point (maybe it should be a separate issue, but I'll put it here for now): The rust parser does not ignore strings on lines like this, even though I put the no-infer-dep directive on the line:

            "runner_module": "tests.test_runner",  # pants: no-infer-dep

Maybe the rust parser should check for the pragma before recording each string, from this: https://github.com/pantsbuild/pants/blob/3632a66bc7918ace25c0aa6f05b4b6a4253d908e/src/rust/engine/dep_inference/src/python/mod.rs#L322-L328 to this:

    fn visit_string(&mut self, node: tree_sitter::Node) -> ChildBehavior {
        let range = node.range();
        let text: &str = self.string_at(range);
        if !!text.contains(|c: char| c.is_ascii_whitespace() || c == '\\') && !self.is_pragma_ignored(node) {
            self.string_candidates
                .insert(text.to_string(), (range.start_point.row + 1) as u64);
        }

[cognifloyd]

@liudonggalaxy Do you get any warnings or errors for your last two points:

2. For the string BASE_PATH = 'a.b.', the Rust parser mistakenly treats it as a dependency, unlike the classic parser.
3. In print('retrying...'), the Rust parser incorrectly interprets it as a dependency, while the classic parser does not.

[liudonggalaxy]

mkdir -p pants_inference pants --python-infer-no-use-rust-parser peek :: > pants_inference/classic_parser.json pants --python-infer-use-rust-parser peek :: > pants_inference/rust_parser.json diff pants_inference/classic_parser.json pants_inference/rust_parser.json

Hello, I didn't observe any warnings or errors, but the following CircleCI test failed in our environment:

mkdir -p pants_inference
pants --python-infer-no-use-rust-parser peek :: > pants_inference/classic_parser.json
pants --python-infer-use-rust-parser peek :: > pants_inference/rust_parser.json
diff pants_inference/classic_parser.json pants_inference/rust_parser.json

To resolve this issue and ensure the CircleCI job passes, we modified the following source code.

BASE_PATH = 'a.b.'
print('retrying...')

->

BASE_PATH ='.'.join(['a', 'b']) + '.'
print('retrying' + '...')

[cognifloyd]

With #20472, only dotted strings that are made up of valid python identifiers will be be considered as possible imports. So, points 2 and 3 should be resolved in pants 2.20 or the 2.19.1 (once released).

To fix the first issue, someone needs to figure out how to deal with concatenated strings with the tree sitter in the dependency parser.