astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33k stars 1.1k forks source link

`ruff analyze graph` does not understand submodule imports #13528

Open minmax opened 1 month ago

minmax commented 1 month ago

ruff 0.6.8

i see something like this:

{
"src/view.py": [
    "src/models/User.py",
  ],
"src/code.py": [
    "src/models/user.py",
  ],
}

content of src/models/__init__.py:

from models.user import User

__all__ = ['User']
charliermarsh commented 1 month ago

Are you able to put together a minimal reproduction, like a set of files I can run ruff over to reproduce? Happy to help.

minmax commented 1 month ago

I was sure it wouldn't work, but fortunately it was easily reproduced.

uvx ruff analyze graph . on https://github.com/minmax/ruff-analyze-tree

  "src/ruff_analyze_tree/colors.py": [
    "src/ruff_analyze_tree/models/User.py"
  ],
  "src/ruff_analyze_tree/models/__init__.py": [
    "src/ruff_analyze_tree/models/user.py"
  ],
MichaReiser commented 1 month ago

Could you explain what result you would expect or why the actual result is incorrect?

minmax commented 1 month ago

The repo don't contains User.py, only user.py, so "src/ruff_analyze_tree/models/User.py" - is invalid.

charliermarsh commented 1 month ago

Maybe the resolver is case insensitive? I mean, that's arguably correct on case-insensitive filesystems which are common.

charliermarsh commented 1 month ago

I can look into it!

minmax commented 1 month ago

Maybe the resolver is case insensitive?

no, im on mac (so case sensitive) and user.py was manually created 30 min ago.

My guess is that it's a Camel case import broke something from models.user import User.

charliermarsh commented 1 month ago

(Just to clarify, it may not matter what filesystem you're on.)

minmax commented 1 month ago

oh my god, "By default, Mac OS uses a case-insensitive file system", im not sure now.

charliermarsh commented 1 month ago

Let's proceed under the assumption that there's a bug here, I will debug.

charliermarsh commented 1 month ago

I do think the issue here is that ruff-analyze-tree/src/ruff_analyze_tree/models/User.py exists from the perspective of the filesystem. \cc @AlexWaygood for insight on whether it's intended in the resolver.

MichaReiser commented 1 month ago

That makes sense. What's the reason for querying the module resolver with User instead of user?

charliermarsh commented 1 month ago

Because the import is from models import User. That could mean that there's a file at models/User.py, so we look for that first. But User is re-exported from models/__init__.py.

charliermarsh commented 1 month ago

(It's incorrect to resolve from models import User to models/user.py; it should map to models/__init__.py. So it's at least incorrect from the perspective of mapping to Python's semantics.)

MichaReiser commented 1 month ago

That's correct, but I think that's an issue in the analyze graph because it first tries to resolve models.User before testing for models

https://github.com/astral-sh/ruff/blob/3018303c8759b3e96d075c62eeb8b8ef24b4d0c3/crates/ruff_graph/src/resolver.rs#L27-L34

Red Knot does the resolution in the opposite order:

  1. First resolve models
  2. Then resolve models.user by looking up User in models
charliermarsh commented 1 month ago

I don't know if I agree... but it depends on how we define the API surface? Like, on the face of it, it's incorrect that the module resolver returns models/User.py given modules.User -- that's not the correct file for that symbol!

As a different example: imagine that models/User.py exists, but models/user.py does not, and we try to resolve import models.user. I think today that would resolve to models/User.py? That seems wrong and has nothing to do with member vs. module imports.

MichaReiser commented 1 month ago

The input of the module resolver isn't the name of a symbol. It's a module path. You have to use red knot's type checker if you want resolution by symbol.

charliermarsh commented 1 month ago

What is a "module path"? Can you explain in terms of the second example I listed?

MichaReiser commented 1 month ago

A module path is a name of a module. E.g. models or models.user

However, the module models.user is not the same as the symbol models.user

From the python spec:

image

The module resolver only implements resolving the module name models.user to a file. It doesn't implement the logic for resolving the symbol user in the scope of models.

For now, what you could do is to use Ruff's existing semantic model to determine if the symbol User exists in models and, if so, don't resolve models.User

@AlexWaygood Is my understanding correct that red knot's inference for from import misses the automatic sub-module fallback?

AlexWaygood commented 1 month ago

@AlexWaygood Is my understanding correct that red knot's inference for from import misses the automatic sub-module fallback?

Yeah, I think that's a known limitation of the red-knot semantic model right now. There's a test case with a TODO for it IIRC. I can look at doing the todo next week!

AlexWaygood commented 1 month ago

The relevant TODOs in red-knot are this comment and these currently-skipped tests. I think we'd fix this in the red-knot semantic model rather than the red-knot module resolver, however, so you might need to implement an independent fix in ruff_graph @charliermarsh

https://github.com/astral-sh/ruff/blob/bee498d6359dd7ce9aa8bff685ca821ab1bbfe31/crates/red_knot_python_semantic/src/types/infer.rs#L1416-L1424

https://github.com/astral-sh/ruff/blob/bee498d6359dd7ce9aa8bff685ca821ab1bbfe31/crates/red_knot_python_semantic/src/types/infer.rs#L2986-L3015