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 an API to resolve the name of a function #68

Open
celinval opened this issue Mar 7, 2024 · 14 comments
Open

Add an API to resolve the name of a function #68

celinval opened this issue Mar 7, 2024 · 14 comments

Comments

@celinval
Copy link
Contributor

celinval commented Mar 7, 2024

It would be really nice to be able to get the definition of a function by its name. In some cases, users may query a tool to perform analysis starting from a specific function. In other cases, tools may want to provide further instrumentation or manipulation such as adding extra assertions, or stubbing a method.

Ideally, the API should be something like:

/// Resolve the name of a function starting from the context of a definition.
fn resolve_function<T: CrateDef>(path: &str, context: T) -> Result<FnDef, Error> { /* */ }

See this discussion here where multiple tool developers describe their current solution:

https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Resolve.20string.20to.20DefId

@celinval
Copy link
Contributor Author

celinval commented Mar 7, 2024

@oli-obk would that make sense?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2024

I dislike it, because it has brought us a lot of trouble, but I also see the need for it. So yea, let's add that.

We should document though that it should not (cannot?) be used for walking into anonymous bodies (closures, const blocks, async blocks)

and we'll need to figure out how to make <Type as Trait>::method nameable

@momvart
Copy link

momvart commented Mar 7, 2024

As a side note, I'm still not okay with limiting the query to functions. Especially, name resolution is quite a general functionality that should be usable for anything.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2024

We cannot hook into name resolution. Name resolution happens too early for us to do lazily. We can walk definitions where possible, but it won't be surface syntax rules in either case.

@celinval
Copy link
Contributor Author

celinval commented Mar 8, 2024

I dislike it, because it has brought us a lot of trouble, but I also see the need for it. So yea, let's add that.

We should document though that it should not (cannot?) be used for walking into anonymous bodies (closures, const blocks, async blocks)

and we'll need to figure out how to make <Type as Trait>::method nameable

I totally get it. We have similar issues with Kani stubs, where we try to mimic name resolution.

@momvart not every definition is supported in StableMIR or MIR, e.g.: modules. That said, we don't need to limit ourselves to functions. We can support other definitions too as long as there's demand for them.

@momvart
Copy link

momvart commented Mar 8, 2024

@momvart not every definition is supported in StableMIR or MIR, e.g.: modules. That said, we don't need to limit ourselves to functions. We can support other definitions too as long as there's demand for them.

It makes sense to me that StableMIR tries to keep definitions as strongly-typed and manageable as possible. However, I feel soon or late there will be some use cases for other definitions as well, and introducing a new function for each is not sustainable. Maybe a structure like the following is better, where the definition types are kept as limited as supported and the queries can be written once but in a more general way.

enum ItemDef {
    Fn(FnDef)
    Static(StaticDef)
    Const(ConstDef)
    // ...
    Other, // For unsupported ones.
}

@oli-obk
Copy link
Contributor

oli-obk commented Mar 8, 2024

Well, we do have https://doc.rust-lang.org/nightly/nightly-rustc/stable_mir/crate_def/struct.DefId.html, so we can return that, and then add support for converting it into more concrete *Defs

@momvart
Copy link

momvart commented Mar 8, 2024

Well, we do have https://doc.rust-lang.org/nightly/nightly-rustc/stable_mir/crate_def/struct.DefId.html, so we can return that

That makes sense, but apparently returning DefId is generally avoided in SMIR as discussed here.

@celinval
Copy link
Contributor Author

celinval commented Mar 8, 2024

That makes sense, but apparently returning DefId is generally avoided in SMIR as discussed here.

We could potentially change that as long as there's a good reason for that.

Question, do you anticipate querying stable mir for something that you don't know the kind and you want to branch accordly? Can you describe your use case a bit more?

@momvart
Copy link

momvart commented Mar 8, 2024

Question, do you anticipate querying stable mir for something that you don't know the kind and you want to branch accordly? Can you describe your use case a bit more?

Not really at the moment and I understand your point that we probably always want to find a particular type of item rather than traversing everything. But let's say there are tools that are interested in finding statics and constants. With the current design:

  • First, they need to request such APIs to be added to MIR with no alternative to fetch items generally.
  • Second, a bunch of similar functions named resolve_* should be added while they merely differ at the filtering part of the query.

In short, as I see smir as a general querying utility to be used in other projects, I would like it to provide general and basic enough APIs for users in case they want to do a very specific query in their tools.

@celinval
Copy link
Contributor Author

celinval commented Mar 8, 2024

One doesn't exclude the other. I propose we create a method that is specific for the type, which is more ergonomic and efficient for the use cases we know exist today.

We can add a more generic way later if there's need. We can always refactor the type specific ones later to invoke the generic code.

@celinval
Copy link
Contributor Author

celinval commented Mar 9, 2024

I would like to avoid only adding a generic method that forces every known user today to match the result for the type they expect.

@momvart
Copy link

momvart commented Mar 9, 2024

We can add a more generic way later if there's need. We can always refactor the type specific ones later to invoke the generic code.

It makes sense. I'm just still not sure if we should add a more generic way backing the type-specific ones now or later when needed.

@celinval
Copy link
Contributor Author

We would need to implement a safe translation from DefId to every possible definition type. I don't think that's high priority since there is no ask for it. So let's focus our limited resources to where it matters the most.

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

No branches or pull requests

3 participants