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

[liudonggalaxy]

Maintainer summary

• find tree sitter incantantion required to handle concatenated strings with [python-infer].string_imports. (issue 1 in the orginal report)
• strings that is not a valid python identifier: a.b. (issue 2 in original report)
• Other strings that are not valid python identifiers retrying... (issue 3 in the orginal report)
• Other strings that are not valid python identifiers (., .., etc) (issue reported in comments)

Original report

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:

pants/src/python/pants/backend/python/dependency_inference/scripts/general_dependency_visitor.py

Lines 26 to 31 in 3da5ae5

	self._string_import_regex = re.compile(
	r"^([a-z_][a-z_\d]*\.){"
	+ os.environ["STRING_IMPORTS_MIN_DOTS"]
	+ r",}[a-zA-Z_]\w*$",
	re.UNICODE,
	)

The new rust parser, however, seems to only exclude strings with a whitespace or a \ char (assuming I'm reading this rust code correctly):

pants/src/rust/engine/dep_inference/src/python/mod.rs

Lines 325 to 328 in 3632a66

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

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:

pants/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py

Lines 117 to 130 in 8eb5557

	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 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)

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:

pants/src/rust/engine/dep_inference/src/python/mod.rs

Lines 322 to 328 in 3632a66

	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.string_candidates
	.insert(text.to_string(), (range.start_point.row + 1) as u64);
	}

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' + '...')