Skip to content

Conversation

@BurntSushi
Copy link
Member

@BurntSushi BurntSushi commented Jul 22, 2025

We want to write queries that depend on Module for caching. While it
seems it can be done without making Module an ingredient, it seems it
is best practice to do so.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Jul 22, 2025
@BurntSushi
Copy link
Member Author

@MichaReiser RE #19408 (comment)

What would be the downside of leaving Module as it was and not making it an ingredient? Is it that dependency tracking in Salsa won't be as good as it would be with this change? I'm trying to figure out what would go wrong if we kept the status quo. (I'm not arguing in favor of the status quo, this is for my own edification.)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@carljm
Copy link
Contributor

carljm commented Jul 22, 2025

I'm curious, given that File:Module is 1:1, why we need to make queries cache based on Module instead of based on its File? AFAIK we've mostly just based everything around File.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the downside of leaving Module as it was and not making it an ingredient? I

The main downside is that all_submodule_names requires the () dummy argument because its (otherwise) only argument (self) isn't a salsa ingredient. What this does behind the scene is that salsa creates an ad-hoc interned struct with two fields: dummy: () and module: self.

I'm curious, given that File:Module is 1:1, why we need to make queries cache based on Module instead of based on its File? AFAIK we've mostly just based everything around File.

I must admit, I didn't look at all_submodule_names in detail and you're right, we could restructure that function so that only the portion operating on File would be the salsa query. That would remove the need to make Module a salsa query today.

It being a salsa struct would still be beneficial if we need queries operating on namespace packages or queries using other Module fields that can't be as easily restructured as all_submodule_names.

@BurntSushi has probably more insight here on whether we want to extend all_submodule_names to namespace packages in the future or if there are other queries that need to operate at the module level

We want to write queries that depend on `Module` for caching. While it
seems it can be done without making `Module` an ingredient, it seems it
is best practice to do so.

[best practice to do so]: #19408 (comment)
This makes caching of submodules independent of whether `Module`
is itself a Salsa ingredient. In fact, this makes the work done in
the prior commit superfluous. But we're possibly keeping it as an
ingredient for now since it's a bit of a tedious change and we might
need it in the near future.

Ref #19495 (review)
@BurntSushi BurntSushi force-pushed the ag/module-ingredient branch from 1ab706b to a5e21ef Compare July 23, 2025 12:53
@BurntSushi
Copy link
Member Author

@BurntSushi has probably more insight here on whether we want to extend all_submodule_names to namespace packages in the future or if there are other queries that need to operate at the module level

I am not 100% certain about this, but I suspect so. I'm thinking about queries for symbols exported by modules (for auto import). If I'm wrong about this and Module is unnecessarily a Salsa ingredient, what kind of downsides are we looking at? Extra memory usage? If it isn't too bad, I think I'd prefer to keep it as an ingredient for now.

I've pushed up another commit that does restructure the code to just use File as an input to a query function.

@MichaReiser
Copy link
Member

If I'm wrong about this and Module is unnecessarily a Salsa ingredient, what kind of downsides are we looking at? Extra memory usage?

  • Accessors are slightly slower as they require a db lookup and they're a bit more annoying to use
  • Salsa has to track additional metadata for each tracked struct. I don't expect this to be a huge problem here as there are relatively few modules (it's not in the millions)

I am not 100% certain about this, but I suspect so. I'm thinking about queries for symbols exported by modules (for auto import).

Makes sense. The main question is if they could be over File instead of Module. But I'm fine merging this as is.

@BurntSushi BurntSushi merged commit cc5885e into main Jul 23, 2025
37 checks passed
@BurntSushi BurntSushi deleted the ag/module-ingredient branch July 23, 2025 13:46
sharkdp added a commit to astral-sh/ty that referenced this pull request Jul 25, 2025
Changes which I chose not to include; let me know if one of these should
be added:

```
- Add warning for unknown `TY_MEMORY_REPORT` value ([#19465](astral-sh/ruff#19465))
- Add goto definition to playground ([#19425](astral-sh/ruff#19425))
- Added support for "go to references" in ty playground. ([#19516](astral-sh/ruff#19516))
- Fall back to `Unknown` if no type is stored for an expression ([#19517](astral-sh/ruff#19517))
- Make `Module` a Salsa ingredient ([#19495](astral-sh/ruff#19495))
- Return a tuple spec from the iterator protocol ([#19496](astral-sh/ruff#19496))
```

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood pushed a commit that referenced this pull request Jul 25, 2025
This makes caching of submodules independent of whether `Module`
is itself a Salsa ingredient. In fact, this makes the work done in
the prior commit superfluous. But we're possibly keeping it as an
ingredient for now since it's a bit of a tedious change and we might
need it in the near future.

Ref #19495 (review)
epitech314 added a commit to epitech314/ty that referenced this pull request Nov 12, 2025
Changes which I chose not to include; let me know if one of these should
be added:

```
- Add warning for unknown `TY_MEMORY_REPORT` value ([#19465](astral-sh/ruff#19465))
- Add goto definition to playground ([#19425](astral-sh/ruff#19425))
- Added support for "go to references" in ty playground. ([#19516](astral-sh/ruff#19516))
- Fall back to `Unknown` if no type is stored for an expression ([#19517](astral-sh/ruff#19517))
- Make `Module` a Salsa ingredient ([#19495](astral-sh/ruff#19495))
- Return a tuple spec from the iterator protocol ([#19496](astral-sh/ruff#19496))
```

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants