ruff icon indicating copy to clipboard operation
ruff copied to clipboard

`ruff analyze graph` does not understand submodule imports

Open minmax opened this issue 1 year ago • 21 comments

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']

minmax avatar Sep 26 '24 15:09 minmax

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

charliermarsh avatar Sep 26 '24 15:09 charliermarsh

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"
  ],

minmax avatar Sep 26 '24 15:09 minmax

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

MichaReiser avatar Sep 26 '24 15:09 MichaReiser

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

minmax avatar Sep 26 '24 16:09 minmax

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

charliermarsh avatar Sep 26 '24 16:09 charliermarsh

I can look into it!

charliermarsh avatar Sep 26 '24 16:09 charliermarsh

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.

minmax avatar Sep 26 '24 16:09 minmax

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

charliermarsh avatar Sep 26 '24 16:09 charliermarsh

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

minmax avatar Sep 26 '24 16:09 minmax

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

charliermarsh avatar Sep 26 '24 16:09 charliermarsh

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.

charliermarsh avatar Sep 26 '24 17:09 charliermarsh

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

MichaReiser avatar Sep 26 '24 18:09 MichaReiser

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 avatar Sep 26 '24 18:09 charliermarsh

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

charliermarsh avatar Sep 26 '24 18:09 charliermarsh

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

MichaReiser avatar Sep 27 '24 07:09 MichaReiser

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.

charliermarsh avatar Sep 27 '24 13:09 charliermarsh

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.

MichaReiser avatar Sep 27 '24 13:09 MichaReiser

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

charliermarsh avatar Sep 27 '24 13:09 charliermarsh

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

  • Module models.user: models is a package (namespace or normal). user can resolve to either a user.py or user/__init__.py
  • Symbol models.user: Import the module models, then resolve the symbol user in its global scope, import the submodule if no such symbol is defined

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?

MichaReiser avatar Sep 27 '24 13:09 MichaReiser

@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 avatar Sep 27 '24 14:09 AlexWaygood

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

AlexWaygood avatar Sep 29 '24 15:09 AlexWaygood