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 CompilerCommand::crate_provides #412

Merged
merged 4 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ members = [
"async-byte-channel",
"benchmark",
"capnpc/test",
"capnpc/test/external-crate",
"capnpc/test-edition-2015",
"capnpc/test-edition-2018",
"capnpc/test-edition-2021",
Expand Down
30 changes: 28 additions & 2 deletions capnpc/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub struct CodeGenerationCommand {
default_parent_module: Vec<String>,
raw_code_generator_request_path: Option<PathBuf>,
capnp_root: String,
crates_provide_map: HashMap<u64, String>,
}

impl Default for CodeGenerationCommand {
Expand All @@ -47,6 +48,7 @@ impl Default for CodeGenerationCommand {
default_parent_module: Vec::new(),
raw_code_generator_request_path: None,
capnp_root: "::capnp".into(),
crates_provide_map: HashMap::new(),
}
}
}
Expand Down Expand Up @@ -93,6 +95,19 @@ impl CodeGenerationCommand {
self
}

/// Sets the crate provides map.
///
/// # Arguments
///
/// - `map` - A map from capnp file id to the crate name that provides the
/// corresponding generated code.
///
/// See [`crate::CompilerCommand::crate_provides`] for more details.
pub fn crates_provide_map(&mut self, map: HashMap<u64, String>) -> &mut Self {
self.crates_provide_map = map;
self
}

/// Generates Rust code according to a `schema_capnp::code_generator_request` read from `inp`.
pub fn run<T>(&mut self, inp: T) -> ::capnp::Result<()>
where
Expand Down Expand Up @@ -210,6 +225,8 @@ impl<'a> GeneratorContext<'a> {
capnp_root: code_generation_command.capnp_root.clone(),
};

let crates_provide = &code_generation_command.crates_provide_map;

for node in ctx.request.get_nodes()? {
ctx.node_map.insert(node.get_id(), node);
ctx.node_parents.insert(node.get_id(), node.get_scope_id());
Expand Down Expand Up @@ -240,8 +257,15 @@ impl<'a> GeneratorContext<'a> {
"{}_capnp",
path_to_stem_string(importpath)?.replace('-', "_")
);
let parent_module_scope = if let Some(krate) = crates_provide.get(&import.get_id())
Copy link
Member

Choose a reason for hiding this comment

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

What should happen if a file in ctx.request.get_requested_files() has this ID too? Then the user is generating code for that file, and it seems to me that the crate-local version should win.

I think that, given an import ID X, the precedence should be:

  1. If the user explicitly requests codegen a file with ID X, then that locally-generated version should be used, regardless of the contents of crates_provide_map.
  2. Otherwise, if X appears in crates_provide_map, then that version of the name should be used.
  3. Otherwise, if the referred-to file has a $Rust.crate annotation, then the Text crate name in that annotation should be used.
  4. Otherwise, compile-time error (or graceful fallback).

I see that you removed the $Rust.crate annotation from this pull request. I do think we should add that annotation. Up to you whether you want it to be in this pull request or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm tempted to say that if a user explicitly says a file should be provided by a crate and also explicitly asks to compile that file themselves we should fail with an error.

My guess is the main reason this would happen is that somebody was confused and thought they had to compile their imports. I can't think of a good reason you'd ever do this.

I agree $Rust.crate should be overridden by the build script option. I removed it because I was thinking it'd only be useful once we had the cargo feature but on second thought I think it's useful right away. I'll add it back.

Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to say that if a user explicitly says a file should be provided by a crate and also explicitly asks to compile that file themselves we should fail with an error.

Yeah that makes sense. I was forgetting that they need to explicitly opt into each individual file in crate_provides().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of fallbacks I don't think it's that important. c++.capnp doesn't have any annotations with non built-in types, so I think it's unlikely users will have such annotations in their code and not want to parse the annotations in Rust.

If it's a field instead of an annotation I can't think of a use case where a fallback would be useful.

Another possible API would be to fail with a helpful error message that suggests adding an explicit exclude_annotations(file_id) call.

Copy link
Member

@dwrensha dwrensha Jun 13, 2023

Choose a reason for hiding this comment

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

I was imagining a schema created for C++ project, where it has a bunch of $Json annotations sprinkled in. A Rust project might not care about JSON and might not want to add a dependency on the capnp_json crate. So I think some way to ignore the annotations is needed. However, since there's probably more discussion to be had about it, and since nobody is actually complaining about it yet, I think it does not belong in the current PR.

{
vec![format!("::{krate}")]
} else {
default_parent_module_scope.clone()
};

ctx.populate_scope_map(
default_parent_module_scope.clone(),
parent_module_scope,
root_name,
NameKind::Verbatim,
import.get_id(),
Expand Down Expand Up @@ -284,7 +308,9 @@ impl<'a> GeneratorContext<'a> {
if annotation.get_id() == NAME_ANNOTATION_ID {
current_node_name = name_annotation_value(annotation)?.to_string();
} else if annotation.get_id() == PARENT_MODULE_ANNOTATION_ID {
ancestor_scope_names = vec!["crate".to_string()];
let head = ancestor_scope_names[0].clone();
ancestor_scope_names.clear();
ancestor_scope_names.push(head);
ancestor_scope_names.append(&mut get_parent_module(annotation)?);
}
}
Expand Down
78 changes: 76 additions & 2 deletions capnpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ pub mod codegen;
pub mod codegen_types;
mod pointer_constants;

use std::path::{Path, PathBuf};
use std::{
collections::HashMap,
path::{Path, PathBuf},
};

// Copied from capnp/src/lib.rs, where this conversion lives behind the "std" feature flag,
// which we don't want to depend on here.
Expand Down Expand Up @@ -119,6 +122,7 @@ pub struct CompilerCommand {
output_path: Option<PathBuf>,
default_parent_module: Vec<String>,
raw_code_generator_request_path: Option<PathBuf>,
crate_provides_map: HashMap<u64, String>,
}

impl CompilerCommand {
Expand Down Expand Up @@ -156,6 +160,75 @@ impl CompilerCommand {
self
}

/// Specify that `crate_name` provides generated code for `files`.
///
/// This means that when your schema refers to types defined in `files` we
/// will generate Rust code that uses identifiers in `crate_name`.
///
/// # Arguments
///
/// - `crate_name`: The Rust identifier of the crate
/// - `files`: the Capnp file ids the crate provides generated code for
///
/// # When to use
///
/// You only need this when your generated code needs to refer to types in
/// the external crate. If you just want to use an annotation and the
/// argument to that annotation is a builtin type (e.g. `$Json.name`) this
/// isn't necessary.
///
/// # Example
///
/// If you write a schema like so
///
/// ```capnp
/// // my_schema.capnp
///
/// using Json = import "/capnp/compat/json.capnp";
///
/// struct Foo {
/// value @0 :Json.Value;
/// }
/// ```
///
/// you'd look at [json.capnp][json.capnp] to see its capnp id.
///
/// ```capnp
/// // json.capnp
///
/// # Copyright (c) 2015 Sandstorm Development Group, Inc. and contributors ...
/// @0x8ef99297a43a5e34;
/// ```
///
/// If you want the `foo::Builder::get_value` method generated for your
/// schema to return a `capnp_json::json_capnp::value::Reader` you'd add a
/// dependency on `capnp_json` to your `Cargo.toml` and specify it provides
/// `json.capnp` in your `build.rs`.
///
/// ```rust,no_run
/// // build.rs
///
/// capnpc::CompilerCommand::new()
/// .crate_provides("json_capnp", [0x8ef99297a43a5e34])
/// .file("my_schema.capnp")
/// .run()
/// .unwrap();
/// ```
///
/// [json.capnp]:
/// https://github.com/capnproto/capnproto/blob/master/c%2B%2B/src/capnp/compat/json.capnp
pub fn crate_provides(
&mut self,
crate_name: impl Into<String>,
files: impl IntoIterator<Item = u64>,
) -> &mut Self {
let crate_name = crate_name.into();
for file in files.into_iter() {
self.crate_provides_map.insert(file, crate_name.clone());
}
self
}

/// Adds the --no-standard-import flag, indicating that the default import paths of
/// /usr/include and /usr/local/include should not bet included.
pub fn no_standard_import(&mut self) -> &mut Self {
Expand Down Expand Up @@ -307,7 +380,8 @@ impl CompilerCommand {
let mut code_generation_command = crate::codegen::CodeGenerationCommand::new();
code_generation_command
.output_directory(output_path)
.default_parent_module(self.default_parent_module.clone());
.default_parent_module(self.default_parent_module.clone())
.crates_provide_map(self.crate_provides_map.clone());
if let Some(raw_code_generator_request_path) = &self.raw_code_generator_request_path {
code_generation_command
.raw_code_generator_request_path(raw_code_generator_request_path.clone());
Expand Down
1 change: 1 addition & 0 deletions capnpc/test-edition-2015/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ capnpc = { path = "../" }
[dependencies]
capnp = { path = "../../capnp" }
capnpc = { path = "../" }
external-crate = { path = "../test/external-crate" }
1 change: 1 addition & 0 deletions capnpc/test-edition-2015/build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
fn main() {
capnpc::CompilerCommand::new()
.crate_provides("external_crate", [0xe6f94f52f7be8fe2])
.file("../test/test.capnp")
.src_prefix("../test/")
.run()
Expand Down
1 change: 1 addition & 0 deletions capnpc/test-edition-2015/test.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
extern crate capnp;
extern crate core;
extern crate external_crate;

pub mod test_capnp {
include!(concat!(env!("OUT_DIR"), "/test_capnp.rs"));
Expand Down
1 change: 1 addition & 0 deletions capnpc/test-edition-2018/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ capnpc = { path = "../" }
[dependencies]
capnp = { path = "../../capnp" }
capnpc = { path = "../" }
external-crate = { path = "../test/external-crate" }
1 change: 1 addition & 0 deletions capnpc/test-edition-2018/build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
fn main() {
capnpc::CompilerCommand::new()
.crate_provides("external_crate", [0xe6f94f52f7be8fe2])
.file("../test/test.capnp")
.src_prefix("../test/")
.run()
Expand Down
1 change: 1 addition & 0 deletions capnpc/test-edition-2021/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ capnpc = { path = "../" }
[dependencies]
capnp = { path = "../../capnp" }
capnpc = { path = "../" }
external-crate = { path = "../test/external-crate" }
1 change: 1 addition & 0 deletions capnpc/test-edition-2021/build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
fn main() {
capnpc::CompilerCommand::new()
.crate_provides("external_crate", [0xe6f94f52f7be8fe2])
.file("../test/test.capnp")
.src_prefix("../test/")
.run()
Expand Down
1 change: 1 addition & 0 deletions capnpc/test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ capnpc = { path = "../" }
[dependencies]
capnp = { path = "../../capnp" }
capnpc = { path = "../" }
external-crate = { path = "./external-crate" }
1 change: 1 addition & 0 deletions capnpc/test/build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
fn main() {
capnpc::CompilerCommand::new()
.crate_provides("external_crate", [0xe6f94f52f7be8fe2])
.file("test.capnp")
.file("in-submodule.capnp")
.file("in-other-submodule.capnp")
Expand Down
12 changes: 12 additions & 0 deletions capnpc/test/external-crate/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "external-crate"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
capnp = { version = "0.17.1", path = "../../../capnp" }

[build-dependencies]
capnpc = { version = "0.17.1", path = "../.." }
6 changes: 6 additions & 0 deletions capnpc/test/external-crate/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn main() {
capnpc::CompilerCommand::new()
.file("external.capnp")
.run()
.expect("compiling schema");
}
7 changes: 7 additions & 0 deletions capnpc/test/external-crate/external.capnp
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@0xe6f94f52f7be8fe2;

annotation annot (*) :Opts;

struct Opts {
field @0 :Text;
}
3 changes: 3 additions & 0 deletions capnpc/test/external-crate/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub mod external_capnp {
include!(concat!(env!("OUT_DIR"), "/external_capnp.rs"));
}
7 changes: 7 additions & 0 deletions capnpc/test/test.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@
@0x99d187209d25cee7;

using Rust = import "rust.capnp";
using External = import "./external-crate/external.capnp";

# The test case is that this builds. This ensure we're able to refer to a struct
# (external_capnp::opts) in the generated code.
struct UseExternalAnnotation $External.annot(field = "foo") {
field @0 :Text;
}

struct FieldSubsetIndexesCorrectly {
common @2 :Text;
Expand Down