Skip to content

analyze: Add option to skip over imports in TYPE_CHECKING blocks #21472

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 16, 2025
Merged

analyze: Add option to skip over imports in TYPE_CHECKING blocks #21472

merged 6 commits into from
Nov 16, 2025

Conversation

@gauthsvenkat
Copy link
Contributor

@gauthsvenkat gauthsvenkat commented Nov 15, 2025

Summary

ruff analyze graph naively detects all imports. This MR adds a feature where TYPE_CHECKING imports can be detected and optionally excluded (using a CLI flag).

Closes: #20736

Main changes

  • [collector.rs] CollectedImport is now a struct instead of an enum. This was done so that a new type_checking field could be added.
  • [collector.rs] Refactored the logic in Stmt::Import{,From} into helper functions collect_import{,_from}. Done so they can be reused in Stmt::If to detect type checking imports.
  • [collector.rs] Added a new helper function is_type_checking_condition. This can detect very simple type-checking conditions, but, practically, it would cover a lot of the cases.

Test Plan

  • Added a new test to check that type-checking imports are detected and filtered, iff --exclude-type-checking-imports flag is set.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 15, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@gauthsvenkat
Copy link
Contributor Author

gauthsvenkat commented Nov 15, 2025 •
edited
Loading

Pipeline fails cause Clippy is complaining there are too many bools in AnalyzeGraphCommand. But it is a CLI option, so not sure how to deal with it.

@AlexWaygood AlexWaygood added the analyze Related to Ruff analyze functionality label Nov 15, 2025
@MichaReiser MichaReiser changed the title [ruff] Option to detect (and exclude) TYPE_CHECKING imports in ruff analyze graph analyze: Add option to skip over imports in TYPE_CHECKING blocks Nov 16, 2025
MichaReiser
MichaReiser requested changes Nov 16, 2025
View reviewed changes
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.

Thank you. I left a few inline comments with a few suggestions

crates/ruff/src/args.rs Outdated
python: Option<PathBuf>,
/// Exclude imports that are only used for type checking (i.e., imports within `if TYPE_CHECKING:` blocks)
#[arg(long)]
exclude_type_checking_imports: bool,
Copy link
Member

@MichaReiser MichaReiser Nov 16, 2025

Choose a reason for hiding this comment

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

I think I'd name this type_checking_only_imports or just type_checking_imports where true includes them and false skips them (should default to true).

We should also add this as a configuration optiont to

ruff/crates/ruff_workspace/src/options.rs

Line 3816 in 6e4b15b

pub struct AnalyzeOptions {

Copy link
Contributor Author

@gauthsvenkat gauthsvenkat Nov 16, 2025

Choose a reason for hiding this comment

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

I've done both.

Adding it as a config option ended up being a bigger change than I expected. I tried to follow how it was done for detect-string-imports. Please let me know if that is good.

Copy link
Contributor Author

@gauthsvenkat gauthsvenkat Nov 16, 2025

Choose a reason for hiding this comment

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

I also just noticed that preview and string_imports config options are not boolean, but instead "enabled" or "disabled". Should I model it that way instead?

Copy link
Member

@MichaReiser MichaReiser Nov 16, 2025

Choose a reason for hiding this comment

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

Yeah, it requires a lot of boilerplate code.

t preview and string_imports config options are not boolean, but instead "enabled" or "disabled". Should I model it that way instead?

I think a boolean is fine.

@gauthsvenkat gauthsvenkat force-pushed the feat/ruff-graph-type-checking-imports branch from 486ab6b to eda7e8b Compare November 16, 2025 10:59
@gauthsvenkat gauthsvenkat force-pushed the feat/ruff-graph-type-checking-imports branch from b681eac to 48a3e66 Compare November 16, 2025 11:34
MichaReiser
MichaReiser reviewed Nov 16, 2025
View reviewed changes
crates/ruff_graph/src/collector.rs
Comment on lines +109 to +110
if self.type_checking_imports || !is_type_checking_condition(test) {
self.visit_body(body);
Copy link
Member

@MichaReiser MichaReiser Nov 16, 2025

Choose a reason for hiding this comment

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

We could consider handling

if !TYPE_CHECKING:
	...
else: 
	...

and

if condition:
	...
elif TYPE_CHECKING:
	...

but we can do this as a separate PR

MichaReiser
MichaReiser approved these changes Nov 16, 2025
View reviewed changes
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.

Thank you

@MichaReiser MichaReiser enabled auto-merge (squash) November 16, 2025 12:29
@MichaReiser MichaReiser merged commit 665f680 into astral-sh:main Nov 16, 2025
34 of 36 checks passed
dcreager added a commit that referenced this pull request Nov 17, 2025
@dcreager
Merge remote-tracking branch 'origin/main' into dcreager/coolable
3e5bff2 
* origin/main: (26 commits)
  Mention `force-exclude` in "Configuration > Python file discovery" (#21500)
  Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418)
  [ty] suppress invalid suggestions in import statements (#21484)
  Limit `eglot-format` hook to eglot-managed Python buffers (#21459)
  Adjust own-line comment placement between branches (#21185)
  [ty] Subscript assignment diagnostics follow-up (#21452)
  [ty] Inlay hint call argument location (#20349)
  [ty] Use `CompactStr` for `StringLiteralType` (#21497)
  Update CodSpeedHQ/action action to v4.3.4 (#21488)
  Update salsa digest to a885bb4 (#21486)
  Update dependency ruff to v0.14.5 (#21489)
  Update astral-sh/setup-uv action to v7.1.3 (#21487)
  Update Rust crate get-size2 to v0.7.2 (#21490)
  Update Rust crate indicatif to v0.18.3 (#21491)
  Update Rust crate quick-junit to v0.5.2 (#21492)
  Update taiki-e/install-action action to v2.62.52 (#21493)
  Fix analyze graph tests on windows (#21481)
  `analyze`: Add option to skip over imports in `TYPE_CHECKING` blocks (#21472)
  [ty] Dataclasses: `__hash__` semantics and `unsafe_hash` (#21470)
  [ty] Dataclass transform: complete set of parameters (#21474)
  ...
@BrewTestBot BrewTestBot mentioned this pull request Nov 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

analyze Related to Ruff analyze functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feature request] option to exclude type checking imports in ruff analyze

3 participants

@gauthsvenkat @MichaReiser @AlexWaygood