Skip to content

Conversation

@MatthewMckee4
Copy link
Contributor

Summary

Don't add subdiagnostic in staticmethod, and change message for classmethod

Resolves astral-sh/ty#584 (partially i think)

Test Plan

Add test and update snapshot

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2025

mypy_primer results

No ecosystem changes detected ✅

@ntBre ntBre added the ty Multi-file analysis & type inference label Jun 5, 2025
carljm
carljm previously requested changes Jun 5, 2025
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.

Thank you!

@MatthewMckee4
Copy link
Contributor Author

I've cleaned this up a bit, but i'm getting panics because of the decorator expression inference. Im not sure what order and function i should call to infer the type of the decorator expression.

@carljm
Copy link
Contributor

carljm commented Jun 10, 2025

A core invariant of our type inference is that each AST node gets a type inferred for it once, in the "inference region" responsible for that node (an inference region can be a scope, a definition, or a standalone expression), and then that type gets merged into the surrounding region as needed, but doesn't get re-inferred willy-nilly.

So I don't think it will work well to have the function_decorators method just take a list of AST nodes; this means we'll end up re-inferring its type ad-hoc from various call-sites, and that won't work well.

The function decorators should have their type inferred exactly once, in infer_function_definition (via infer_decorator). We shouldn't call self.infer_expression or similar on those same nodes anywhere else.

The previous code here that checked for abstract methods and overloads used self.file_expression_type on those decorator nodes, which doesn't re-infer them: it looks up what scope they belong to, infers that entire scope, and then pulls the expression types out. This worked, but isn't really ideal because it infers far more than necessary. A function definition is always a definition, and the decorator nodes are part of that definition, so we don't need to infer the entire scope, just the definition.

So the ideal here would be to find the Definition for the function (scopes know the Definition that creates them, so this shouldn't be hard to get from the body scope) and then use definition_expression_type to infer types for that definition and pull the specific inferred expression types out. (Although it seems like we store on the OverloadLiteral for the function what known decorators it has, so if we get the type for the function definition we can maybe just use that and not need to pull out specific decorator expressions at all?

@MatthewMckee4
Copy link
Contributor Author

i've not yet used definition_expression_type, but there's also tests failing and i can't see what regression ive made

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 12, 2025

CodSpeed Instrumentation Performance Report

Merging #18487 will not alter performance

Comparing MatthewMckee4:incorrect-subdiagnostic-for-unresolved-reference (ba648f9) with main (57bd7d0)

Summary

✅ 39 untouched benchmarks

@AlexWaygood AlexWaygood requested a review from carljm June 14, 2025 13:07
MatthewMckee4 and others added 3 commits June 14, 2025 16:42
…ence

* main: (71 commits)
  Bump 0.12.0 (astral-sh#18724)
  Revert "[ty] Offer "Did you mean...?" suggestions for unresolved `from` imports and unresolved attributes (astral-sh#18705)" (astral-sh#18721)
  [`flake8-return`] Stabilize only add `return None` at the end when fixing `implicit-return` (`RET503`) (astral-sh#18516)
  [`pyupgrade`] Stabilize `non-pep695-generic-function` (`UP047`) (astral-sh#18524)
  [`pyupgrade`] Stabilize `non-pep695-generic-class` (`UP046`) (astral-sh#18519)
  [`pandas-vet`] Deprecate `pandas-df-variable-name` (`PD901`) (astral-sh#18618)
  [`flake8-bandit`] Remove `suspicious-xmle-tree-usage` (`S320`) (astral-sh#18617)
  Stabilize `dataclass-enum` (`RUF049`) (astral-sh#18570)
  Stabilize `unnecessary-dict-index-lookup` (`PLR1733`) (astral-sh#18571)
  Remove rust-toolchain.toml from sdist (astral-sh#17925)
  Stabilize `starmap-zip` (`RUF058`) (astral-sh#18525)
  [`flake8-logging`] Stabilize `exc-info-outside-except-handler` (`LOG014`) (astral-sh#18517)
  [`pyupgrade`] Stabilize `non-pep604-annotation-optional` (`UP045`) and preview behavior for `non-pep604-annotation-union` (`UP007`) (astral-sh#18505)
  Stabilize `pytest-warns-too-broad` (`PT030`) (astral-sh#18568)
  Stabilize `for-loop-writes` (`FURB122`) (astral-sh#18565)
  Stabilize `pytest-warns-with-multiple-statements` (`PT031`) (astral-sh#18569)
  Stabilize `pytest-parameter-with-default-argument` (`PT028`) (astral-sh#18566)
  Stabilize `nan-comparison` (`PLW0177`) (astral-sh#18559)
  Stabilize `check-and-remove-from-set` (`FURB132`) (astral-sh#18560)
  Stabilize `unnecessary-round` (`RUF057`) (astral-sh#18563)
  ...
@AlexWaygood
Copy link
Member

There's a surprisingly large mypy_primer diff on this PR right now

@carljm
Copy link
Contributor

carljm commented Jun 17, 2025

Yeah, the mypy primer diff definitely seems to indicate something unexpected is happening here. Don't have time to investigate it further right now, will have to come back to this later.

@sharkdp sharkdp marked this pull request as draft June 27, 2025 07:52
@sharkdp
Copy link
Contributor

sharkdp commented Jun 27, 2025

Moving this to draft for now, feel free to move it back to review whenever you think it's ready for another round of review.

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 is great. It looks like the primer hits have all gone now that #18809 has landed on main, so I think this is good to go now!

@AlexWaygood AlexWaygood marked this pull request as ready for review June 27, 2025 12:00
@AlexWaygood AlexWaygood dismissed carljm’s stale review June 27, 2025 12:02

requested changes were made

@AlexWaygood AlexWaygood force-pushed the incorrect-subdiagnostic-for-unresolved-reference branch from 86935ad to 452b39d Compare June 27, 2025 12:36
@AlexWaygood AlexWaygood force-pushed the incorrect-subdiagnostic-for-unresolved-reference branch from 452b39d to ba648f9 Compare June 27, 2025 12:36
@AlexWaygood AlexWaygood added bug Something isn't working diagnostics Related to reporting of diagnostics. labels Jun 27, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) June 27, 2025 12:38
@AlexWaygood AlexWaygood merged commit a3c79d8 into astral-sh:main Jun 27, 2025
37 checks passed
dcreager added a commit that referenced this pull request Jun 27, 2025
* main:
  [ty] Add builtins to completions derived from scope (#18982)
  [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487)
  [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981)
  [ty] Add micro-benchmark for #711 (#18979)
  [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974)
  [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975)
  [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963)
  [`pylint`] Fix `PLC0415` example (#18970)
  [ty] Add environment variable to dump Salsa memory usage stats (#18928)
  [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678)
  Bump 0.12.1 (#18969)
  [`FastAPI`] Add fix safety section to `FAST002` (#18940)
  [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965)
  [ty] Resolve python environment in `Options::to_program_settings` (#18960)
  [`ruff`] Fix false positives and negatives in `RUF010` (#18690)
  [ty] Fix rendering of long lines that are indented with tabs
  [ty] Add regression test for diagnostic rendering panic
  [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)
@MatthewMckee4 MatthewMckee4 deleted the incorrect-subdiagnostic-for-unresolved-reference branch June 28, 2025 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working diagnostics Related to reporting of diagnostics. ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect subdiagnostic suggestions for unresolved-reference diagnostics

5 participants