define cfg_if! and cfg_if_expr! internal macros #438

cosmicexplorer · 2025-09-30T15:18:11Z

Problem

The zip crate occasionally encounters some very tricky platform-specific conditional logic. Because Rust's #[cfg(...)] markers are only parsed and not typechecked, it's very easy to introduce subtle breakage that only shows up in a specific scenario.

Performing some action if #[cfg(...)] condition is true and another is not is a common desire, but #[cfg(not(...))] clauses to achieve this tend to be difficult to read and audit.

Solution

There is an existing crate cfg-if which provides a very simple macro_rules! macro to perform these if/else conditional checks safely, but we would like to avoid introducing an external dependency that controls the very fine-grained distinctions between platforms. So upon seeing that the stdlib also just vendors in the cfg_if! macro, this PR does the same for our crate:

cfg_if! {
  if #[cfg(...)] {
    ...
  } else {
    ...
  }
}

However, the Rust syntax has a subtle and very annoying failure mode that disallows annotating certain expressions with #[cfg(...)] except on nightly compilers. In particular, this makes cfg_if! unusable for expression-based platform conditionals. So a second macro_rules! macro was also devised named cfg_if_expr!, which wraps the result of cfg_if! within an immediately-invoked closure. Using this macro looks like:

cfg_if_expr! {
  #[cfg(...)] => f(),
  #[cfg(...)] => g(),
  _ => h(), // the "else" branch
}

Result

Like most macros, the result is aesthetic and somewhat subjective. I think that these macros serve the security goals of this crate by incorporating an explicit else or _ => branch. I think the negative logic of #[cfg(not(...))] clauses is extremely difficult to model for readers of the code, and since the two #[cfg(...)] and #[cfg(not(...))] branches aren't linked syntactically, they may eventually drift away from each other over time.

Using these macros makes it clear that the code will match exactly one case, evaluated in linear order. By standardizing that single-branch scenario, we make it easier to focus our analysis on the unusual cases that don't or can't conform to this simpler model.

amazon-q-developer · 2025-09-30T15:18:17Z

⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion.

Using Amazon Q Developer for GitHub

Amazon Q Developer¹ is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation.

Slash Commands

Command	Description
`/q <message>`	Chat with the agent to ask questions or request revisions
`/q review`	Requests an Amazon Q powered code review
`/q help`	Displays usage information

Features

Agentic Chat
Enables interactive conversation with Amazon Q to ask questions about the pull request or request specific revisions. Use /q <message> in comment threads or the review body to engage with the agent directly.

Code Review
Analyzes pull requests for code quality, potential issues, and security concerns. Provides feedback and suggested fixes. Automatically triggered on new or reopened PRs (can be disabled for AWS registered installations), or manually with /q review slash command in a comment.

Customization

You can create project-specific rules for Amazon Q Developer to follow:

Create a .amazonq/rules folder in your project root.
Add Markdown files in this folder to define rules (e.g., cdk-rules.md).
Write detailed prompts in these files, such as coding standards or best practices.
Amazon Q Developer will automatically use these rules when generating code or providing assistance.

Example rule:

All Amazon S3 buckets must have encryption enabled, enforce SSL, and block public access.
All Amazon DynamoDB Streams tables must have encryption enabled.
All Amazon SNS topics must have encryption enabled and enforce SSL.
All Amazon SNS queues must enforce SSL.

Feedback

To provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository.

For more detailed information, visit the Amazon Q for GitHub documentation.

Amazon Q Developer uses generative AI. You may need to verify generated code before using it in your environment. See the AWS Responsible AI Policy. ↩

gemini-code-assist · 2025-09-30T15:18:35Z

Summary of Changes

Hello @cosmicexplorer, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request integrates custom cfg_if! and cfg_if_expr! macros into the zip crate to streamline and clarify conditional compilation. By replacing disparate #[cfg] blocks with these structured macros, the change aims to enhance code readability, simplify auditing of platform-specific code paths, and mitigate the risk of subtle bugs arising from complex #[cfg(not(...))] logic. The cfg_if_expr! macro specifically addresses challenges in applying #[cfg] to expressions on stable Rust.

Highlights

Introduced cfg_if! macro: A new internal macro for structured conditional compilation, providing explicit if/else branches to improve readability and auditability of platform-specific logic, similar to the cfg-if crate and stdlib's internal usage.
Introduced cfg_if_expr! macro: A new internal macro designed for expression-based conditional compilation, circumventing Rust's #[cfg] limitations on expressions for stable Rust by using immediately-invoked closures.
Refactored conditional logic: Existing #[cfg] blocks across various files (examples/write_dir.rs, src/compression.rs, src/read.rs, src/types.rs, src/zipcrypto.rs) have been updated to utilize these new macros, standardizing the approach to platform-specific code.
Added macros module: A new src/macros.rs file was added to house these internal utility macros, making them available to internal project dependencies while being hidden from external users.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

amazon-q-developer

Summary

This PR introduces internal cfg_if! and cfg_if_expr! macros to improve platform-specific conditional compilation logic. The implementation is well-structured and addresses a real need for cleaner conditional compilation patterns.

Key Findings

Critical Issues:

Crash Risk: The todo!() macro in src/read.rs will panic on non-Unix/Windows platforms when processing symlinks, creating a potential denial-of-service vulnerability
Behavioral Change: Using std::process::exit(1) in examples changes program flow compared to the original return 1 statements

Suggestions:

The macros need proper re-exports at the crate root for external accessibility
Remove unnecessary clippy suppressions where possible
Consider separating unrelated changes (like the fuzzing → arbitrary feature change) into separate commits

Recommendation

The core macro implementation is solid and improves code maintainability. However, the critical crash risk with todo!() must be addressed before merging. The behavioral changes in examples should also be reviewed to ensure they maintain the intended program flow.

The PR title correctly follows Conventional Commits format with the feat: prefix, which is appropriate for this new internal functionality.

src/lib.rs

src/macros.rs

examples/write_dir.rs

src/zipcrypto.rs

src/read.rs

gemini-code-assist

Code Review

This pull request introduces cfg_if! and cfg_if_expr! macros to simplify platform-specific conditional logic, removing verbose #[cfg] attributes throughout the codebase. This is a great improvement for code readability and maintainability. The implementation of the macros is solid, vendoring cfg_if and adding a new cfg_if_expr for expression-based conditionals.

My review focuses on ensuring the refactoring doesn't introduce regressions. I've found one high-severity issue where symlink handling on non-Unix/Windows platforms was changed to panic, which is a regression from the previous behavior of treating them as regular files. I've also noted a minor clippy-related style issue. Overall, this is a valuable change once the identified issues are addressed.

src/read.rs

            let mut file = self.by_index(i)?;

            let mut outpath = directory.clone();
+            /* TODO: the control flow of this method call and subsequent expectations about the


cosmicexplorer · 2025-09-30T16:32:07Z

src/read.rs

+                drop(file);
                make_symlink(&outpath, &target, &self.shared.files)?;
                continue;
            }
-            let mut file = self.by_index(i)?;


This method body in general is the only place that has any non-trivial conditional logic changed. I believe the if within the #[cfg(any(...))] block is easier to follow. I've also changed all #[cfg(...)] conditionals to rest directly on the if block they adorn, instead of the prior convention which created a larger enclosing scope for each #[cfg(...)] clause.

But I particularly want to point out the one non-trivial logic change in the region I've highlighted here. The reason we needed to drop(file) was because of lifetime requirements for self on the call to make_symlink(). However, the previous code was unconditionally dropping the current entry handle on every single iteration.

This is very unlikely to have any noticeable effect on performance now, but it seems more correct and could be necessary for performance of future strategies. Since it's a very subtle semantic change, I wanted to call it out.

- allow unreachable code in a branch of a cfg_if_expr! - make main() alone handle process exit status - refactor files_by_unix_mode - allow specifying output type for cfg_if_expr! - use cfg_if! and cfg_if_expr! to rewrite a complex conditional for calculating default compression - add explicit compile_error!() cases to catch subtle fallback scenarios

Pr0methean

Looks good; just a few nitpicks.

Pr0methean · 2025-10-08T23:01:36Z

src/macros.rs

+///
+/// Note that any `return` operation within the expressions provided to this macro will apply to the
+/// generated closure, not the enclosing scope--it cannot be used to interfere with external
+/// control flow.


Can this gotcha be checked at build-time, by raising an error when a branch expression starts with return, break or continue and a warning when those keywords occur later in the expression?

Pr0methean · 2025-10-08T23:02:45Z

src/read.rs

-            CryptoReader::Aes {
-                vendor_version: AesVendorVersion::Ae2,
-                ..
+        cfg_if! {


Looks like the needless_return can probably be eliminated here by using cfg_if_expr! instead.

Pr0methean · 2025-10-08T23:05:15Z

src/read.rs

+/// Store all entries which specify a numeric "mode" which is familiar to POSIX operating systems.
+#[cfg(unix)]
+#[derive(Default, Debug)]
+struct FilesByUnixMode {


Call this UnixModesByPath instead. "By" usually refers to a map's keys, not values.

Pr0methean · 2025-10-08T23:06:51Z

src/read.rs

+impl FilesByUnixMode {
+    pub fn add_mode(&mut self, path: PathBuf, mode: u32) {
+        // We don't print a warning or consider it remotely out of the ordinary to receive two
+        // separate modes for the same path: just take the later one.


Why does this happen? It's not clear to me that this is the correct behavior, especially in debug builds.

amazon-q-developer bot reviewed Sep 30, 2025

View reviewed changes

src/lib.rs Show resolved Hide resolved

src/macros.rs Show resolved Hide resolved

examples/write_dir.rs Show resolved Hide resolved

src/zipcrypto.rs Show resolved Hide resolved

src/read.rs Show resolved Hide resolved

src/read.rs Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Sep 30, 2025

View reviewed changes

src/read.rs Outdated Show resolved Hide resolved

src/read.rs Show resolved Hide resolved

cosmicexplorer force-pushed the cfg-if-internal branch 2 times, most recently from 744947c to 98de40b Compare September 30, 2025 15:54

github-advanced-security bot found potential problems Sep 30, 2025

View reviewed changes

cosmicexplorer commented Sep 30, 2025

View reviewed changes

cosmicexplorer force-pushed the cfg-if-internal branch from 08cef35 to 3764d59 Compare September 30, 2025 16:36

cosmicexplorer requested a review from Pr0methean September 30, 2025 16:50

cosmicexplorer force-pushed the cfg-if-internal branch 3 times, most recently from 8deaa94 to 33ab21b Compare October 1, 2025 05:08

Pr0methean requested changes Oct 8, 2025

View reviewed changes

define cfg_if! and cfg_if_expr! internal macros #438

Are you sure you want to change the base?

define cfg_if! and cfg_if_expr! internal macros #438

Uh oh!

Conversation

cosmicexplorer commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Result

Uh oh!

amazon-q-developer bot commented Sep 30, 2025

Slash Commands

Features

Customization

Feedback

Footnotes

Uh oh!

gemini-code-assist bot commented Sep 30, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

amazon-q-developer bot left a comment

Choose a reason for hiding this comment

Summary

Key Findings

Recommendation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Check notice

cosmicexplorer Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pr0methean left a comment

Choose a reason for hiding this comment

Uh oh!

Pr0methean Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pr0methean Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Pr0methean Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Pr0methean Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cosmicexplorer commented Sep 30, 2025 •

edited

Loading

cosmicexplorer Sep 30, 2025 •

edited

Loading

Pr0methean Oct 8, 2025 •

edited

Loading