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

Add public function for resolving callable AST exprs to their HIR equivalents #16705

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

regexident
Copy link
Contributor

(the PR is motivated by an outside use of the ra_ap_hir crate that would benefit from being able to walk a hir::Function's AST, resolving callable exprs within to their HIR equivalents)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2024
Comment on lines 206 to 211
pub fn resolve_method_call_expr(
&self,
method_call_expr: &ast::MethodCallExpr,
) -> Option<Function> {
self.imp.resolve_method_call(method_call_expr)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is already exposed via deref, we should re-export SemanticsImpl from the crate root so it and the deref impl for Semantics gets properly documented

Copy link
Contributor Author

@regexident regexident Feb 28, 2024

Choose a reason for hiding this comment

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

Oh, gotcha. Removed and #16707 opened for SemanticsImpl.

db: &dyn HirDatabase,
call: &ast::CallExpr,
) -> Option<Callable> {
self.type_of_expr(db, &call.clone().into())?.0.as_callable(db)
Copy link
Member

Choose a reason for hiding this comment

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

This returns the return type as a callable, not the function itself. resolve_call seems unnecessary as you can just do sema.type_of_expr(call_expr.expr()).as_callable() (with Option handling) as is. (method calls are special because there is no nested expression that corresponds to the resolved function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yeah. I saw it used that way, but as a user of the public API without intricate knowledge of ra_ap_hir's internals the type_of_expr is rather non-obvious and thus easy to miss (anecdotally it took me quite a bit of searching before I found the pattern). While it doesn't strictly add any functionality that wasn't available before it makes it discoverable and thus more accessible to crate API users.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wouldn't really say that that's related to our hir api at all, as this is effectively how rust works! A call expression is a call on some receiver expression, so in my eyes it's only logical to me that you'd query the callable on the receiver. Though I suppose we could have a general expr_as_callable(expr: &ast::Expr) -> Option<Callable> function for discoverability and parity with the method_call one.

Copy link
Contributor Author

@regexident regexident Feb 28, 2024

Choose a reason for hiding this comment

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

Good point, fn resolve_expr_as_callable it is then! Updated.

@Veykril
Copy link
Member

Veykril commented Feb 29, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 29, 2024

📌 Commit ed34978 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 29, 2024

⌛ Testing commit ed34978 with merge ecda464...

@bors
Copy link
Collaborator

bors commented Feb 29, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ecda464 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Feb 29, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ecda464 to master...

@bors bors merged commit ecda464 into rust-lang:master Feb 29, 2024
11 checks passed
@bors
Copy link
Collaborator

bors commented Feb 29, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@regexident regexident deleted the resolve-callable-exprs branch February 29, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants