Skip to content

THRIFT-6059: add crate_prefix option to Rust codegen for cross-file imports#3546

Open
santiagomed wants to merge 1 commit into
apache:masterfrom
santiagomed:THRIFT-6059-rs-crate-prefix-option
Open

THRIFT-6059: add crate_prefix option to Rust codegen for cross-file imports#3546
santiagomed wants to merge 1 commit into
apache:masterfrom
santiagomed:THRIFT-6059-rs-crate-prefix-option

Conversation

@santiagomed

@santiagomed santiagomed commented May 27, 2026

Copy link
Copy Markdown
Contributor

Problem

The Rust code generator hardcodes use crate:: for cross-file imports (e.g., fileA.thrift includes fileB.thrift generates use crate::fileB;). This assumes the generated files are placed at the crate root (src/). When files are placed in a submodule (e.g., src/thrift/), the correct prefix is super::, and users must post-process generated files with sed.

Fix

Add a crate_prefix generator option that controls the path prefix. Default remains crate for backward compatibility.

# Files at crate root — existing behavior, unchanged
thrift --gen rs -out src/ schema.thrift

# Files in a submodule
thrift --gen rs:crate_prefix=super -out src/thrift/ schema.thrift

# Nested module paths also work
thrift --gen rs:crate_prefix=crate::generated -out src/generated/ schema.thrift

Changes

  • t_rs_generator constructor reads crate_prefix from parsed_options
  • crate_prefix_ member stores the value (default: "crate")
  • Import generation in render_attributes_and_includes uses crate_prefix_ instead of hardcoded "crate"
  • THRIFT_REGISTER_GENERATOR updated to document the option
  • An empty value (rs:crate_prefix=) is rejected: it would silently emit use ::module;, which is an external-crate path in Rust 2018+
  • Unknown options are reported as unknown option rs:<name>, matching the convention used by other generators

Testing

Verified manually with a two-file schema (event.thrift including trees.thrift):

Invocation Result
--gen rs use crate::trees; (unchanged)
--gen rs:crate_prefix=super use super::trees;
--gen rs:crate_prefix=crate::generated use crate::generated::trees;
--gen rs:crate_prefix= error: crate_prefix requires a non-empty value
--gen rs:bogus=1 error: unknown option rs:bogus

There is no existing test harness for rs generator options (lib/rs/test_recursive exercises cross-file imports with the default prefix only), so no automated test is included; happy to add one if reviewers have a preferred location.


Portions of this change were AI-assisted (Grok 4.3, xAI) and reviewed by the author.

@mergeable mergeable Bot added rust Pull requests that update Rust code compiler labels May 27, 2026
@santiagomed

Copy link
Copy Markdown
Contributor Author

@mhlakhani @Jens-G could I get a review here please? Also open to hearing other solutions to this issue with submodules!

@Jens-G

Jens-G commented Jun 9, 2026

Copy link
Copy Markdown
Member

Code review

Found 1 issue:

  1. The new option-parsing loop throws "unknown option: " + iter->first, but every other generator in the codebase uses a language-prefixed form (e.g., "unknown option py:", "unknown option cpp:"). The RS generator should throw "unknown option rs:" + iter->first to follow the established convention and help users identify which generator rejected their flag.

std::map<std::string, std::string>::const_iterator iter;
for (iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
if (iter->first.compare("crate_prefix") == 0) {
crate_prefix_ = iter->second;
} else {
throw "unknown option: " + iter->first;
}

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

…mports

Client: rs

The Rust code generator hardcodes 'use crate::' for cross-file imports
(e.g., when event.thrift includes trees.thrift). This assumes the
generated files are placed at the crate root (src/). When files are
placed in a submodule (e.g., src/thrift/), the correct prefix is
'super', and users must post-process with sed.

Add a crate_prefix generator option: --gen rs:crate_prefix=super

Default remains 'crate' for backward compatibility. An empty value is
rejected since it would silently emit 'use ::module;' (an external
crate path in Rust 2018+). Unknown options are reported with the
generator prefix ('unknown option rs:') per compiler convention.

Co-Authored-By: Grok 4.3 <noreply@x.ai>
@santiagomed santiagomed force-pushed the THRIFT-6059-rs-crate-prefix-option branch from 4ce4898 to 320c6bd Compare June 10, 2026 02:31
@santiagomed

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. The new option-parsing loop throws "unknown option: " + iter->first, but every other generator in the codebase uses a language-prefixed form (e.g., "unknown option py:", "unknown option cpp:"). The RS generator should throw "unknown option rs:" + iter->first to follow the established convention and help users identify which generator rejected their flag.

std::map<std::string, std::string>::const_iterator iter;
for (iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
if (iter->first.compare("crate_prefix") == 0) {
crate_prefix_ = iter->second;
} else {
throw "unknown option: " + iter->first;
}

🤖 Generated with Claude Code

  • If this code review was useful, please react with 👍. Otherwise, react with 👎.

Thanks for the review @Jens-G. Addressed this + added some additional fixes specified in the summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants