Skip to content

Comments

red-knot: move symbol lookups in symbol.rs#16152

Merged
dhruvmanila merged 2 commits intomainfrom
dhruv/symbol-api
Feb 17, 2025
Merged

red-knot: move symbol lookups in symbol.rs#16152
dhruvmanila merged 2 commits intomainfrom
dhruv/symbol-api

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 14, 2025 •
edited
Loading

Summary

This PR does the following:

  • Moves the following from types.rs in symbol.rs:
    • symbol
    • global_symbol
    • imported_symbol
    • symbol_from_bindings
    • symbol_from_declarations
    • SymbolAndQualifiers
    • SymbolFromDeclarationsResult
  • Moves the following from stdlib.rs in symbol.rs and removes stdlib.rs:
    • known_module_symbol
    • builtins_symbol
    • typing_symbol (only for tests)
    • typing_extensions_symbol
    • builtins_module_scope
    • core_module_scope
  • Add symbol_from_bindings_impl and symbol_from_declarations_impl to keep RequiresExplicitReExport an implementation detail
  • Make declaration_type a pub(crate) as it's required in symbol_from_declarations (binding_type is already pub(crate)

The main motivation is to keep the implementation details private and only expose an ergonomic API which uses sane defaults for various scenario to avoid any mistakes from the caller. Refer to #16133 (comment), #16133 (comment) for details.

@dhruvmanila dhruvmanila added the ty Multi-file analysis & type inference label Feb 14, 2025
@dhruvmanila dhruvmanila force-pushed the dhruv/symbol-api branch 2 times, most recently from 0220b09 to 59aef97 Compare February 14, 2025 03:50
@dhruvmanila dhruvmanila marked this pull request as ready for review February 14, 2025 04:35
@dhruvmanila dhruvmanila mentioned this pull request Feb 14, 2025
Merged
Base automatically changed from dhruv/re-export-3 to main February 14, 2025 09:55
AlexWaygood
AlexWaygood approved these changes Feb 14, 2025
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! It's great to move these out of types.rs.

Bikeshedding: I'm wondering if we should put these in a new submodule (lookup.rs?) rather than symbol.rs. The APIs in symbol.rs are currently quite abstract and don't have much to do with the business of implementing Python's lookup semantics correctly; these APIs we're moving here feel a little different from that. I don't have a strong opinion, however; I'm happy to go with this

We could also add some more doc-comments to the module_type_symbol function, as I mentioned in #16133 (comment)

carljm
carljm approved these changes Feb 14, 2025
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 good to me! I agree with all of Alex's comments.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Feb 17, 2025

Bikeshedding: I'm wondering if we should put these in a new submodule (lookup.rs?) rather than symbol.rs. The APIs in symbol.rs are currently quite abstract and don't have much to do with the business of implementing Python's lookup semantics correctly; these APIs we're moving here feel a little different from that. I don't have a strong opinion, however; I'm happy to go with this

We added a LookupResult in symbol.rs in #16160 which makes me wonder if we should rename symbol.rs to lookup.rs instead.

But, I also see this comment on LookupResult:

ruff/crates/red_knot_python_semantic/src/symbol.rs

Lines 176 to 177 in 89cefbe

/// Note that this type is exactly isomorphic to [`Symbol`].
/// In the future, we could possibly consider removing `Symbol` and using this type everywhere instead.

which makes me hesitant to do it now and can be done easily in the future as well.

@dhruvmanila dhruvmanila merged commit 9f111ea into main Feb 17, 2025
21 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/symbol-api branch February 17, 2025 12:15
dcreager added a commit that referenced this pull request Feb 18, 2025
@dcreager
Merge branch 'main' into alex/eager-scopes
5db470e 
* main: (60 commits)
  [`refurb`] Manual timezone monkeypatching (`FURB162`) (#16113)
  [`pyupgrade`] Do not upgrade functional TypedDicts with private field names to the class-based syntax (`UP013`) (#16219)
  Improve docs for PYI019 (#16229)
  Refactor `CallOutcome` to `Result` (#16161)
  Fix minor punctuation errors (#16228)
  Include document specific debug info (#16215)
  Update server to return the debug info as string (#16214)
  [`airflow`] Group `ImportPathMoved` and `ProviderName` to avoid misusing (`AIR303`) (#16157)
  Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values (#16187)
  Ignore source code actions for a notebook cell (#16154)
  Add FAQ entry for `source.*` code actions in Notebook (#16212)
  red-knot: move symbol lookups in `symbol.rs` (#16152)
  better error messages while loading configuration `extend`s (#15658)
  Format `index.css` (#16207)
  Improve API exposed on `ExprStringLiteral` nodes (#16192)
  Update Rust crate tempfile to v3.17.0 (#16202)
  Update cloudflare/wrangler-action action to v3.14.0 (#16203)
  Update NPM Development dependencies (#16199)
  Update Rust crate smallvec to v1.14.0 (#16201)
  Update Rust crate codspeed-criterion-compat to v2.8.0 (#16200)
  ...
@mishamsk mishamsk mentioned this pull request Feb 19, 2025
Merged
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

@MichaReiser MichaReiser Awaiting requested review from MichaReiser

@sharkdp sharkdp Awaiting requested review from sharkdp

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dhruvmanila @carljm @AlexWaygood