Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 17, 2025
Merged

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

merged 2 commits into from
Feb 17, 2025

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 14, 2025

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 red-knot 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
Base automatically changed from dhruv/re-export-3 to main February 14, 2025 09:55
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)

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

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:

/// 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
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants