Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

# pants: no-infer-dep not always respected by the 2.17 Rust parser #19751

Closed
lowvoltage opened this issue Sep 1, 2023 · 4 comments · Fixed by #19293 or #19804
Closed

# pants: no-infer-dep not always respected by the 2.17 Rust parser #19751

lowvoltage opened this issue Sep 1, 2023 · 4 comments · Fixed by #19293 or #19804
Assignees
Labels
backend: Python Python backend-related issues bug
Milestone

Comments

@lowvoltage
Copy link
Contributor

Describe the bug
The new Rust inference parser disrespects a # pants: no-infer-dep on a conditional import:

if TYPE_CHECKING:
    from a import ClassA  # pants: no-infer-dep

Dependencies' diff below:

❯ pants --python-infer-no-use-rust-parser peek main.py
[
  {
    "address": "//main.py",
    "target_type": "python_source",
    "dependencies": [],
    "dependencies_raw": null,
❯ pants --python-infer-use-rust-parser peek main.py
[
  {
    "address": "//main.py",
    "target_type": "python_source",
    "dependencies": [
      "//a.py"
    ],
    "dependencies_raw": null,
    "description": null,

Note and workarounds
This seems to break only when # pants: no-infer-dep is on the last line inside the if block:

  • With mulitple import statements, only the last one is handled incorrectly
  • A multiline import seems to work fine
    from a import (
        ClassA,  # pants: no-infer-dep
    )
  • A placeholder pass statement at the end of the if TYPE_CHECKING block also results in a correct behavior

Pants version
2.17.0

OS
Both: MacOS and Linux

Additional info
Minimalist setup to reproduce

main.py

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from a import ClassA  # pants: no-infer-dep

print('Hello, world!')

a.py

class ClassA:
    pass

pants.toml

[GLOBAL]
pants_version = "2.17.0"
backend_packages = ["pants.backend.python"]

[python]
interpreter_constraints = ["==3.9.*"]

BUILD

python_sources()
@lowvoltage lowvoltage added the bug label Sep 1, 2023
@thejcannon thejcannon self-assigned this Sep 1, 2023
thejcannon added a commit that referenced this issue Sep 1, 2023
This change is split out of
#19751

Specifically the goal would be to use a different resolve for the
plugins so that we model "external plugins" correctly in the Pants repo
(which we should be doing anyways)
@huonw huonw added the backend: Python Python backend-related issues label Sep 1, 2023
@huonw huonw added this to the 2.17.x milestone Sep 5, 2023
@huonw
Copy link
Contributor

huonw commented Sep 7, 2023

Thanks for taking the time to file an issue.

As an additional work around that you may be aware of, you can continue using the non-Rust parser, i.e. set use_rust_parser = false instead of true. The Rust parser is new and somewhat experimental, so thanks for helping shake out the bugs.

@thejcannon
Copy link
Member

OK, oddly adding

  assert_imports(
    r"
    from typing import TYPE_CHECKING

    if TYPE_CHECKING:
        from a import ClassA  # pants: no-infer-dep

    print('Hello, world!')",
    &["typing.TYPE_CHECKING"],
  );

to src/rust/engine/dep_inference/src/python/tests.rs results in... tests passing.

@thejcannon
Copy link
Member

Oh, but I'm on main...

if I checkout 2.17.x it fails.

So I think #19293 fixed this. I'll get it cherry picked backwards.

Kudos @lilatomic

huonw added a commit that referenced this issue Sep 8, 2023
#19293) (#19788)

fixes #17691 and fixes #19751

also:
- fixes the case of nested weakenings
- bumps tree-sitter-python to an unreleased version (last release was a
year ago)
- rebuilds the generation of tree-sitter node types to support multiple
symbols for a name

Co-authored-by: Daniel Goldman <[email protected]>
Co-authored-by: Huon Wilson <[email protected]>
thejcannon added a commit that referenced this issue Sep 8, 2023
#19293) (#19789)

fixes #17691 and fixes #19751

also:
- fixes the case of nested weakenings
- bumps tree-sitter-python to an unreleased version (last release was a
year ago)
- rebuilds the generation of tree-sitter node types to support multiple
symbols for a name

---------

Co-authored-by: Daniel Goldman <[email protected]>
huonw added a commit that referenced this issue Sep 8, 2023
This closes #19751 by adding some tests for it. We're pretty sure the
real fix was in #19293, but only incidentally, so this PR makes sure
we've got a regression test specifically for #19751.

I've marked the test for cherrypicking so that we confirm that this is
fixed in the earlier releases too.
WorkerPants pushed a commit that referenced this issue Sep 8, 2023
This closes #19751 by adding some tests for it. We're pretty sure the
real fix was in #19293, but only incidentally, so this PR makes sure
we've got a regression test specifically for #19751.

I've marked the test for cherrypicking so that we confirm that this is
fixed in the earlier releases too.
WorkerPants pushed a commit that referenced this issue Sep 8, 2023
This closes #19751 by adding some tests for it. We're pretty sure the
real fix was in #19293, but only incidentally, so this PR makes sure
we've got a regression test specifically for #19751.

I've marked the test for cherrypicking so that we confirm that this is
fixed in the earlier releases too.
huonw added a commit that referenced this issue Sep 9, 2023
…-pick of #19804) (#19807)

This closes #19751 by adding some tests for it. We're pretty sure the
real fix was in #19293, but only incidentally, so this PR makes sure
we've got a regression test specifically for #19751.

I've marked the test for cherrypicking so that we confirm that this is
fixed in the earlier releases too.

Co-authored-by: Huon Wilson <[email protected]>
huonw added a commit that referenced this issue Sep 9, 2023
…-pick of #19804) (#19808)

This closes #19751 by adding some tests for it. We're pretty sure the
real fix was in #19293, but only incidentally, so this PR makes sure
we've got a regression test specifically for #19751.

I've marked the test for cherrypicking so that we confirm that this is
fixed in the earlier releases too.

Co-authored-by: Huon Wilson <[email protected]>
@lilatomic
Copy link
Contributor

I'm not sure what part of #19293 fixes this. I think it might be the newer version of the tree-sitter grammar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues bug
Projects
None yet
4 participants