Skip to content

Fix CachedMapper's rec/rec_function_definition typing#654

Merged
inducer merged 2 commits into
inducer:mainfrom
majosm:fix-cached-mapper-rec-typing
Jun 3, 2026
Merged

Fix CachedMapper's rec/rec_function_definition typing#654
inducer merged 2 commits into
inducer:mainfrom
majosm:fix-cached-mapper-rec-typing

Conversation

@majosm

@majosm majosm commented Mar 26, 2026

Copy link
Copy Markdown
Collaborator

Eliminates the basedpyright warnings that appear every time a cached mapper overrides rec/rec_function_definition, e.g.:

pytato/transform/__init__.py:529:44 - warning: Type of "rec" is partially unknown
  Type of "rec" is "(self: Mapper[Unknown, Unknown, ...], expr: Array | AbstractResultWithNamedArrays, ...) -> Unknown" (reportUnknownMemberType)
pytato/transform/__init__.py:529:44 - warning: Argument type is unknown
  Argument corresponds to parameter "result" in function "_cache_add" (reportUnknownArgumentType)
pytato/transform/__init__.py:542:25 - warning: Type of "rec_function_definition" is partially unknown
  Type of "rec_function_definition" is "(self: Mapper[Unknown, Unknown, ...], expr: FunctionDefinition, ...) -> Unknown" (reportUnknownMemberType)
pytato/transform/__init__.py:542:25 - warning: Argument type is unknown
  Argument corresponds to parameter "result" in function "_function_cache_add" (reportUnknownArgumentType)

by using super(CachedMapper, self) instead of Mapper.

Testing out Claude for type checking troubleshooting. 🙂 cc @lukeolson

@majosm majosm marked this pull request as ready for review March 26, 2026 16:12
@majosm majosm requested a review from inducer March 26, 2026 16:17
@majosm majosm force-pushed the fix-cached-mapper-rec-typing branch from 953e246 to 646a66b Compare March 26, 2026 18:37
@inducer inducer requested a review from Copilot April 3, 2026 14:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses recurring basedpyright warnings around CachedMapper subclasses overriding rec/rec_function_definition by adjusting how the base implementations are invoked so that typing information is preserved while still avoiding double-caching.

Changes:

  • Switch CachedMapper’s internal delegation from explicit Mapper.rec(...) / Mapper.rec_function_definition(...) calls to super().rec(...) / super().rec_function_definition(...).
  • Update selected CachedMapper-derived overrides to use super(CachedMapper, self).rec(...) to bypass CachedMapper.rec and prevent double caching without triggering basedpyright “unknown member type” warnings.
  • Remove corresponding entries from the basedpyright baseline now that the warnings are resolved.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
pytato/transform/metadata.py Updates AxisTagAttacher.rec to bypass CachedMapper.rec via super(CachedMapper, self) and tightens local typing assertions.
pytato/transform/__init__.py Adjusts CachedMapper delegation to super() for improved typing; refines function-definition dispatch typing; updates comments to document the double-caching hazard and correct bypass pattern.
pytato/analysis/__init__.py Updates TagCountMapper.rec to bypass CachedMapper.rec via super(CachedMapper, self) to avoid double caching with improved typing.
.basedpyright/baseline.json Removes baseline suppressions corresponding to the fixed typing warnings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@inducer inducer force-pushed the fix-cached-mapper-rec-typing branch from 646a66b to f6320c3 Compare June 3, 2026 21:07
@inducer inducer force-pushed the fix-cached-mapper-rec-typing branch from f6320c3 to aba89bf Compare June 3, 2026 21:20
@inducer inducer merged commit a4258e3 into inducer:main Jun 3, 2026
10 checks passed
@inducer

inducer commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Thx!

@inducer

inducer commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants