Expand allowed queries

[simolus3]

This is work in progress - I've applied the changes to _analyzer_macros, if the general direction matches what we want I'll also migrate _cfe_macros and adopt this in the json macro so that we can test this.

This expands the queries sent from macros to the query service to allow more fine-grained control about what gets resolved:

1. The existing Query type is renamed to QueryName. It is meant to resolve an entire class.
2. I've also added QueryStaticType, which also resolves an interface type by name but does nothing more. This is useful for macros which want to do type queries on fixed classes (e.g. "Is this field a subtype of List? If so, what kind of List<T>?" - this requires having List in the type hierarchy, but those macros don't need to know what methods List has).
3. I've also added QueryMultiple, which allows sending multiple nested queries in a single Query, saving roundtrips if macros know what they need beforehand).

Query is then a union of these three query types.

One question we might want to discuss is whether QueryName and QueryStaticType need to be separate types - I think the functionality to only query about some aspect of a class will be very useful, but maybe we can also add fields to QueryName that describe what information is desired.

[jakemac53]

In general, I think the query types need to be split based on the phases, and probably the names should just be a direct indication of the phase they are available in and less about what they are allowed to do.

That allows us to expand the capabilities later on without feeling the need to change the name, since it has no immediate semantics attached to the name itself.

So, TypePhaseQuery, DeclarationPhaseQuery, DefinitionPhaseQuery etc. Which all build on each other.

[simolus3]

I think I understand what you're saying, and I agree with it, but I had a really hard time coming up with actual queries for the individual phases (we can't really port the Introspector methods from the old API over to queries since the model is too different, and the current query type doesn't fall into the phases at all). Is this still the up-to-date information on phases and what's available in them?

As an example for what I'm struggling with: What's our equivalent of TypePhaseIntrospector.resolveIdentifier? Pretty much what we currently have in Query since it takes a uri/name pair? But Query can also resolve everything in a class, so it currently covers resolveIdentifier and most of the declaration-phase introspection capabilities as well. Should we split this into separate types? Or couldn't the query service just return less information for the same query depending on the phase it was issued in?

Some things are more obvious (we obviously can't do type-hierarchy queries in phase 1), but I couldn't find any precise information on what introspection queries are available in which phase. My mental model is essentially this:

1. Phase 1: Unresolved AST for (uri, name) pair
2. Phase 2: Previous + Type hierarchy, + Listing all types in a library, + Resolved AST (names resolved to definitions) for (uri, name) pair
3. Phase 3: Previous + Listing all definitions in a library

Is that roughly it? Should all these be different query types?

[jakemac53]

Yes, I definitely agree it is not clear how we will map the new query model to phases. But we do need to push on that. It is very much an open question right tnow.

Worst case we can enforce on the server side that queries aren't asking for more than they are allowed to, but that seems less than ideal.

Is this still the up-to-date information on phases and what's available in them?

Yes

[davidmorgan]

My guess is that what we end up with is a flat set of queries and metadata about which phases they're allowed in.

Then the host can use that information to check what's valid. (It should not assume client side validation; that's more trouble than it's worth).

And for the macro we can build some helper(s) from the phase bucketing, so it's easy to construct only queries that are valid in the current phase.

[simolus3]

I've added an association securely mapping incoming QueryRequests to pending AugmentationRequests on the host. This allows checking whether a given query is valid or not, and I've added a check forbidding type-related queries in phase 1.

Does this address the concerns? It creates a mapping for queries to the active macro phase. If we have more types of queries in the future, we can expand the server-side validation logic.

And for the macro we can build some helper(s) from the phase bucketing, so it's
easy to construct only queries that are valid in the current phase.

Yeah if we have different methods on Macro for the different phases we could pass a different introspector to each of them, similar to how it's done in the SDK macro API.

[jakemac53]

Then the host can use that information to check what's valid. (It should not assume client side validation; that's more trouble than it's worth).

If the query types were actually specific to the phase, only valid queries would be possible :) (no validation at all on either side is needed).

[simolus3]

(no validation at all on either side is needed)

So should there be a TypePhaseQueryRequest, DeclarationsPhaseQueryRequest and DefinitionsPhaseQueryRequest?
Still, without verification, what stops a macro from sending a DeclarationsPhaseQueryRequest while serving a phase one AugmentationRequest?

[jakemac53]

Somewhat related to #91 - I want to bring the current APIs much more in line with what we have today in the SDK, where things are a lot more locked down by the APIs themselves.

So, a special introspection API would be provided which only exposes the ability to do the kinds of queries that are allowed, as an example.

However, this is purely aspirational at the moment. It isn't at all clear how to actually lock down the query API such that only those things are possible, while keeping it general enough to do everything macros need.

[simolus3]

I'll follow that issue then and keep this PR on hold until we have a resolution. One motivation for this format was the desire to reduce roundtrips between the macro host and the client (hence the batch query, for example). I imagine that property won't be easy to maintain with typed introspection APIs.

[simolus3]

Closing since it doesn't match the updated introspection APIs.