get_annotation_types breaks on non-builtin external types, despite comment in codegen.rs saying otherwise

[dzfranklin]

The comment says "Avoid referring to the annotation in the generated code, so that users can import annotation schemas like c++.capnp or rust.capnp without needing to generate code for them, as long as the annotations are not generic." But the generated code does refer to annotation types.

This breaks if the annotation has a non-builtin type defined externally.

capnproto-rust/capnpc/src/codegen.rs

Line 1662 in 39a5248

// Avoid referring to the annotation in the generated code, so that users can import

For example

using Json = import "/capnp/compat/json.capnp";

struct SimpleNamedUnion {
  value :union $Json.discriminator(name = "renamed-value", valueName = "type") {
    unset @0 :Void;
    variant @1 :UInt8;
  }
}

generates the following, which won't compile

    pub fn get_annotation_types(child_index: Option<u16>, index: u32) -> ::capnp::introspect::Type {
        match (child_index, index) {
          (Some(0), 0) => <crate::json_capnp::discriminator_options::Owned as ::capnp::introspect::Introspect>::introspect(),
          _ => panic!("invalid annotation indices ({:?}, {}) ", child_index, index),
        }
    }

The simplest solution I can think of that doesn't require users to do anything but works for all annotations would be for us to codegen the necessary the annotation types in a private module in each file generated. That adds a little duplication to save users from having to include_str! a new file, which I think is probably worthwhile. We can reduce the bloat by passing a flag to codegen_node telling it to skip generating the Reader & Builder.

[dwrensha]

This potentially relates to #233. You could imagine that the capnp_json crate might be the canonical place for the json.capnp schema to live, but then downstream crates will want a way to import that schema.

[dzfranklin]

This potentially relates to #233.

I think annotations are only used in the .capnp files of downstream consumers, never in their rust code. capnp_json is wholly responsible for the rust code which reads JSON annotations. So it's not important for consumers to get the generated rust code, just the .capnp file. And copy-pasting that is not a huge deal.

We also want to be able to ignore annotations we don't care about. I shouldn't need to add a capnp_cpp crate just to be able to compile a capnproto schema that is shared between Rust and C++ projects and uses C++ annotations with non-builtin types.

What if we made get_annotation_types return an AnyPointer for structs? Because as json_capnp I think I can rely on annotation 0xcfa794e8d19a0162, for example, being a struct compatible with DiscriminatorOptions so I can just cast at read time. If a library makes a backwards-incompatible change to an annotation or a user defines an annotation with an identical id to an existing one they're asking for trouble. This is a minor breaking change I think.

Edit: I forgot how the api to cast works, you'd need to match on the type first. So it's a major breaking change to anyone using get_annotation_types on struct options. But I think it's relatively reasonable to assume I'm the first one doing that because of the issue I ran into.

[dwrensha]

What if we made get_annotation_types return an AnyPointer for structs?

That would make it significantly less useful, and would break the kind of usage that's shown off in the fill_random_values example:

capnproto-rust/example/fill_random_values/fill.capnp

Lines 28 to 32 in ae6eb67

	struct SelectFrom(T) {
	annotation choices(field) : T;
	# Selects from a set of choices. `T` should be a list type `List(S)` where `S`
	# is the type of the field being annotated.
	}

capnproto-rust/example/fill_random_values/shapes.capnp

Lines 13 to 14 in ae6eb67

	# Try adding this annotation to the Color fields below:
	# $Fill.SelectFrom(List(Color)).choices(.palette);

[dzfranklin]

You're right, I forgot about generics. I'll see if I can do #233 then.

[dwrensha]

You're right, I forgot about generics.

Generics are the whole reason that we need get_annotation_types() and get_field_types().

I'll see if I can do #233 then.

In the meantime, as a stopgap, maybe codegen.rs can detect cases that are going to fail to compile and it could just exclude those branches? Maybe it could even add an error message like "you need to generate code for foo.capnp" in those branches.

For example, when it sees that it needs a type from json.capnp, and json.capnp is not a file that has been explicitly requested for codegen, it would omit that branch.

[dwrensha]

Or, to adapt your idea from earlier, it could return AnyPointer in those cases.

[dwrensha]

Closing because #412 has landed and has been released in capnpc-v0.17.2