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.

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

@no_type_check support #15122

Merged
merged 9 commits into from
Dec 30, 2024
Merged

@no_type_check support #15122

merged 9 commits into from
Dec 30, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Dec 23, 2024 •
edited
Loading

Summary

This PR adds support for @no_type_check. Specifically, functions annotated with @no_type_check.

I think I solved it and the below no longer applies :)

The basics are working, but I'm running into cycles when the diagnostics are emitted from the annotated function's signature and I want to get a second opinion on what the best approach.

The current implementation infers the FunctionType and then checks if any decorator in FunctionLiteralType::decorators is the known NoTypeCheck function. This works well, but cycles are more likely because FunctionLiteralType infers the type of the return type and decorators, and any of those could create a cycle.

An alternative is to do something closer to Pyright:
Instead of resolving the entire FunctionLiteralType, iterate over the function's decorators directly and infer each decorator expression. This reduces the possible cycles because it reduces the scope to the decorators only.

Either approach requires a recovery_fn on the called query to return Type::Unknown for the decorator's expression. So I think this PR might
be blocked on fixpoint iteration.

Test Plan

Added tests, some of them panic :)

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Dec 23, 2024
AlexWaygood
AlexWaygood reviewed Dec 24, 2024
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! This overall seems like the way I would have gone about implementing this.

I think I understand the issue you point out with regard to cycles -- correct me if I'm wrong:

  1. This PR means that we need to evaluate the types for certain functions eagerly, in order to determine if they're decorated with @no_type_check, because we need to know if a function is decorated with @no_type_check in order to determine whether we should be emitting diagnostics regarding that function
  2. But we emit diagnostics during the process of type inference, so some types aren't actually ready yet at the point when we emit diagnostics

Examining the types of the decorators directly (without inferring the type of the function that's being decorated) makes sense to me... or potentially we could filter out the @no_type_check-ignored diagnostics after the whole file has been inferred (rather than as part of context.report_lint()), because at that point we know that all types are "ready"?

@MichaReiser

This comment was marked as resolved.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 24, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

carljm
carljm approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great! Nice solution to the cycle issues.

AlexWaygood
AlexWaygood approved these changes Dec 24, 2024
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice!

@MichaReiser MichaReiser enabled auto-merge (squash) December 30, 2024 09:37
@MichaReiser MichaReiser merged commit 0caab81 into main Dec 30, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/no-type-check branch December 30, 2024 09:42
dcreager added a commit that referenced this pull request Dec 30, 2024
@dcreager
Merge branch 'main' into dcreager/infer-binary
dffd23d 
* main:
  Add all PEP-585 names to UP006 rule (#5454)
  [`flake8-simplify`] More precise inference for dictionaries (`SIM300`) (#15164)
  `@no_type_check` support (#15122)
  Visit PEP 764 inline `TypedDict`s' keys as non-type-expressions (#15073)
  [red-knot] Add diagnostic for invalid unpacking (#15086)
  [`flake8-use-pathlib`] Catch redundant joins in `PTH201` and avoid syntax errors (#15177)
  Update Rust crate glob to v0.3.2 (#15185)
  Update astral-sh/setup-uv action to v5 (#15193)
  Update dependency mdformat-mkdocs to v4.1.1 (#15192)
  Update Rust crate serde_with to v3.12.0 (#15191)
  Update NPM Development dependencies (#15190)
  Update pre-commit hook rhysd/actionlint to v1.7.5 (#15189)
  Update Rust crate syn to v2.0.93 (#15188)
  Update Rust crate serde to v1.0.217 (#15187)
  Update Rust crate quote to v1.0.38 (#15186)
  Update Rust crate compact_str to v0.8.1 (#15184)
  [`flake8-type-checking`] Disable TC006 & TC007 in stub files (#15179)
  Test explicit shadowing involving `def`s (#15174)
  Fix typo in `NameImport.qualified_name` docstring (#15170)
  [airflow]: extend names moved from core to provider (AIR303) (#15159)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carljm carljm carljm approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

Assignees
No one assigned
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MichaReiser @carljm @AlexWaygood