Skip to content

Fix CachedMapper's rec/rec_function_definition typing#654

Open
majosm wants to merge 2 commits intoinducer:mainfrom
majosm:fix-cached-mapper-rec-typing
Open

Fix CachedMapper's rec/rec_function_definition typing#654
majosm wants to merge 2 commits intoinducer:mainfrom
majosm:fix-cached-mapper-rec-typing

Conversation

@majosm
Copy link
Copy Markdown
Collaborator

@majosm majosm commented Mar 26, 2026

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
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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.

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.

2 participants