Skip to content

Conversation

MichaReiser
Copy link
Member

Summary

Using HashMap::raw_entry is error prone. There are so many ways to hold the API wrong.
Instead, use the new HashTable type that avoids those footguns.

Test Plan

cargo test

@MichaReiser MichaReiser added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Jun 20, 2025
Copy link
Contributor

github-actions bot commented Jun 20, 2025

mypy_primer results

No ecosystem changes detected ✅

@MichaReiser MichaReiser force-pushed the micha/place-table-hash-table branch from 9b59ceb to e2752b7 Compare June 20, 2025 10:01
@MichaReiser MichaReiser force-pushed the micha/place-table-hash-table branch from e2752b7 to e7db49d Compare June 20, 2025 10:02
@AlexWaygood AlexWaygood removed their request for review June 20, 2025 10:24
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I'd prefer if @carljm could take a quick look as well as he's probably the one most familiar with the place.rs code as it was mainly refactored largely to add attribute / subscript expression support.

Copy link
Member

@dcreager dcreager left a comment

Choose a reason for hiding this comment

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

New HashTable API (TIL) looks much easier to use, thank you!

@MichaReiser MichaReiser merged commit f544026 into main Jun 20, 2025
36 checks passed
@MichaReiser MichaReiser deleted the micha/place-table-hash-table branch June 20, 2025 13:31
@carljm
Copy link
Contributor

carljm commented Jun 20, 2025

Looks great, thank you! I remember in the first version of this code, we did hold the API wrong, and ended up with a symbol table that broke as soon as the internal hash table resized 😆

dcreager added a commit that referenced this pull request Jun 20, 2025
* main: (21 commits)
  [`flake8-logging`] Avoid false positive for `exc_info=True` outside `logger.exception` (`LOG014`) (#18737)
  [`flake8-pie`] Small docs fix to `PIE794` (#18829)
  [`pylint`] Ignore __init__.py files in (PLC0414) (#18400)
  Avoid generating diagnostics with per-file ignores (#18801)
  [`flake8-simplify`] Fix false negatives for shadowed bindings  (`SIM910`, `SIM911`) (#18794)
  [ty] Fix panics when pulling types for `ClassVar` or `Final` parameterized with >1 argument (#18824)
  [`pylint`] add fix safety section (`PLR1714`) (#18415)
  [Perflint] Small docs improvement to `PERF401` (#18786)
  [`pylint`] Avoid flattening nested `min`/`max` when outer call has single argument (`PLW3301`) (#16885)
  [`ruff`] Added `cls.__dict__.get('__annotations__')` check (`RUF063`) (#18233)
  [ty] Use `HashTable` in `PlaceTable` (#18819)
  docs: Correct collections-named-tuple example to use PascalCase assignment (#16884)
  [ty] ecosystem-analyzer workflow (#18719)
  [ty] Add support for `@staticmethod`s (#18809)
  unnecessary_dict_kwargs doc - a note on type checking benefits (#18666)
  [`flake8-pytest-style`] Mark autofix for `PT001` and `PT023` as unsafe if there's comments in the decorator (#18792)
  [ty] Surface matched overload diagnostic directly (#18452)
  [ty] Report when a dataclass contains more than one `KW_ONLY` field (#18731)
  [`flake8-pie`] Add fix safety section to `PIE794` (#18802)
  [`pycodestyle`] Add fix safety section to `W291` and `W293`  (#18800)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants