Avoiding cloning with pico

[rbalicki2]

Right now, we clone unnecessarily a lot! (look for .note_todo("Do not clone. Use a MemoRef."). There are 48 results. Yeesh.)

The issue is that pico used to not work with MemoRef's, and the alternative was extensive cloning. Now, we've caused it to work well with MemoRef, so we can go back and clone less.

I want to sketch the general plan for how we might minimize cloning for client selectables.

Principles

• Functions whose result changes often (or whose results change superfluously) should not be called from functions that are expensive to run.

Client selectables

Client selectables are created from client declarations. (Client declarations are created from extractions, which are created from the source text, but that's not relevant). Let's assume for this issue that we want to keep that structure, though it can probably be improved. We want to minimize cloning and maximize reuse of values.

There are two needs:

• to access an individual selectable, and
• to iterate over every selectable

Accessing an individual selectable (identified by (entity name, selectable name), which we call a selectable_id) is not so easy. We don't know where it's defined, for example! (And it might be defined in multiple locations, invalidly).

So, working backwards, there must be a function that does the bare minimum of parsing of each input file. The result of this parsing is a client declaration. So

fn get_declaration_map(db) -> HashMap<SelectableId, Declaration> {
  db.get_all_file_contents().iter().map(...).collect()
}

(There are no Result<T, E> in this hash map! If we can't parse a given extraction, we simply ignore it. What else can we do? See below.)

Now, the result of calling this function will change superfluously. Changes to unrelated files will cause it to have a different value. So, we need an intervening function to cut down on the superfluous changes:

fn get_declarations(db, selectable_id) -> 
  Option<MemoRef<Vec<Declaration>>>
{
  db.intern_ref(get_declaration_map(db).get(selectable_id))
}

Okay, great, the results of this function will only change if the declaration has changed. That's good!

But, what should the return value be? Should it be a MemoRef? Alas no. MemoRef seems like the right move, but it's actually wrong, because if the map changes, we will have to return a new memo ref, i.e. it doesn't allow us to short circuit very well. But that's what we want!

So, we probably want to clone. :/

fn get_declarations(db, selectable_id) -> 
  Option<Vec<Declaration>>
{
  get_declaration_map(db).get(selectable_id).cloned()
}

So, we have to keep at least two copies of each declaration in memory. Not the end of the world, but really, not ideal. What else can we do here?

• we can try to make the "parse the minimal amount of everything" step even more minimal.

Anyway, to be continued.

[rbalicki2]

Okay, continuing to think out loud here. My new thinking is this:

• there are at least three namespaces: entities, selectables (for a given entity), and aliases at a given level in a given selection set. (The last one is more complicated and less important.)
  • there are more! Variables, for example. But anyway, less fundamental and less important.
• for a given namespace, there is an entity type (Entity, Selectable, Selection)
• everything here is "for a given entity type":

---

• we should parse the original text as permissively as possible and include locations.
  • this data structure should be generic over TLocation and TParseError.
  • That's because we can almost immediately get rid of locations, and that will improve memoization. So every field on this struct has With<Result<...>, TLocation>, and at first TLocation = Location, later TLocation = ().
  • We also want to ensure that no parse errors occurred at some point, so TParseError can go from Diagnostic to ! (i.e. the never type).
  • each entity needs to have a .map_location(f) function, or at least a .drop_location fn to do this conversion, and a unwrap_results fn.
  • note that unwrapping parse errors is not sufficient! We have non-parse errors, which e.g. relate to relationships. e.g. a server selectable might point to an undefined entity. In addition, untyped directives need to be parsed into a specific struct.
  • I'm not sure what to do about those "derived errors", but I have some ideas. Anyway, this is a problem for the future and for now, we can just validate all of them in validate.rs.
• the only fields that must succeed in parsing are the identifier + how to determine the type of the entity, because if those didn't parse correctly, there's no way to partially use that entity.
• there are no additional structs, except the location-free and location-and-parse-error-free version!
• so, TNetworkProtocol::parse_type_system_documents should actually return something like (Vec<SchemaScalarEntity<Location, Diagnostic>>, Vec<SchemaObjectEntity<Location, Diagnostic>>, Vec<Selectable<Location, Diagnostic>>) (see below **), i.e. there shouldn't be a distinction between the declaration and the in-memory entity. And there should be no distinction between client declarations and client selectables.

---

More principals, from a different direction:

• Because we don't know in what file a declaration lives, the first layer (***) must always parse everything and return something like

DiagnosticResult<
  Vec<
    With<
      DiagnosticResult<Item<...>>, 
      Location
    >
  >
>

with the outer result being anything fatal (say, schema file not existing).

• the next layer should always accept the identifier and return the subset of items that have a particular identifier. (Note that we skip the entities that failed to parse + skip the ones with parse errors for identifiers, and return an empty vec if e.g. the schema file doesn't exist): Vec<With<Item<...>, Location>>
• the next layer should always return the single entity named X in a given namespace, i.e. Option<With<Item<...>, Location>>. There is a namespace for entities, for selectables (for a given object entity) and for selection aliases (for a given selectable, at a given level in the selection set, for a given type of selection set: reader or refetch). (We can probably ignore selection sets for now.) This fn should accept the namespace id (() or parent_object_entity_id) + id within namespace (parent_object_entity_id or selectable_name).
• at this point, we can have another layer that calls .drop_location on the struct -> Option<With<..., ()>>
• this gives us the struct with parse errors for individual fields, and this is the canonical function that we use for everything pre-generating artifacts
• this canonical struct is actually an enum (see below)
• the assumption at this point is that an entity is "likely not to change much", i.e. we don't necessarily need to memoize much past the entity. But that's why selection sets (volatile!) and client selectables (much less volatile) should be different entities.
• for generating artifacts + validation, we should possibliy use the function get_xxx_with_no_parse_errors_named (but like... does it matter? I dunno.)

---

Notes on server scalar/object entities

• For the scalar/object entities, the fields with parse results and locations are description + directives
• javascript_name and concrete_type are weird! They probably belong on associated types. (javascript_name belongs on something like TargetPlatformSchemaEntityAssociatedData, but we have no TTargetPlatform generic.) concrete_type already can be derived from the network protocol associated type data.

---

Enums?

• get_selectable_named(db, parent_object_entity_name, selectable_name) should return a four-way enum: server/client + scalar/object.
• from here we have six more methods: get_client_selectable_named, get_client_scalar_selectable_named etc
• that part's easy!
• but in order to call get_selectable_named, we actually need to depend on two sets of input vecs: server selectables and client selectables. that fusion should occur at the first level (marked (***)).

---

Iterating over all entities in a given namespace or all entities of a given type

• We need to do this when validating, at that point, we can just use the first level (***) function, or rather the first-level function but with dropped locations + no parse errors (i.e. surfaced parse errors)
• We need to do this if we generate a GraphQL schema containing all of the client selectables
• We don't need to do this to add asFoo fields, since that's within TNetworkProtocol::parse_type_system_documents
• In autocomplete, we can also use the first-level functions (though de-duplicated!)

---

What about compiler-generated client fields like __link and expose-as fields?

• Well, every location is optional: it can be Location::Generated, so we should ... be good on locations. But maybe we want to encode that more explicitly in the types. I don't know!
• (**) exposeAs fields... well, the parse_type_system_documents fn should return client fields, too! Specifically, these. Is there anything stopping us from doing this? (And thus it should also return selection sets.) I don't think so. I think it's fine.

---

What happens if certain steps fail?

• Consider the GraphQL schema not existing. Can we get a selectable named Query.Foo, e.g. for goto definition? (e.g. if it's a top-level selection on a query on type Query)
• Yeah, we can! But we might incorrectly go to one definition, instead of showing all of them. (goto definition calls get_selectables_named(...) not get_one_selectable_named(...), because it can surface a list of results)
• We thus need to guard against this when generating artifacts! But that's what validate.rs (jankily) does, as well as get_only_xxx_with_no_parse_errors_named

[rbalicki2]

New thought: the network protocol fn should return a vec of non-blocking (for LSP) errors, which can include "this field was defined twice." But what it returns to isograph land is a hashmap of entity name -> one single entity. That simplifies a lot of things! Later, in validate.rs, we ensure that that vec of errors is empty before writing artifacts.

(I am not convinced of this. Is having multiple locations for goto definition a good thing, if something is defined multiple times...? I mean, maybe? That might happen if, for example, you have your schema generated from multiple sources.)