Skip to content

feat: wdk_build::TraitsMap for collection of implemented traits for bindgen output - #652

Open
Leon Durrenberger (leon-xd) wants to merge 12 commits into
microsoft:mainfrom
leon-xd:derives-library
Open

feat: wdk_build::TraitsMap for collection of implemented traits for bindgen output#652
Leon Durrenberger (leon-xd) wants to merge 12 commits into
microsoft:mainfrom
leon-xd:derives-library

Conversation

@leon-xd

@leon-xd Leon Durrenberger (leon-xd) commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

This PR introduces wdk_build::TraitsMap, a struct to collect the traits each bindgen emitted type implements. See #654 for the related PR.

Purpose

These changes are a pre-requisite to solve the current issues with generating bindings for mutually exclusive WDK headers (tracked by #516 ). The solution requires types and constants for subsystems to be generated in independent bindgen runs, and will be published in a follow-up PR. In order to prevent duplicate type generation, bindgen invocation for subsystem types will utilize a blocklist_file for every file traversed in the initial base types run.

bindgen by default assumes very minimal derived traits when encountering a blocklisted type, so this implementation on its own would leave most subsystem types with a sparse set of automatic trait implementations. This is a regression that is unacceptable. In order to prevent this while still keeping our mutual exclusive headers solution, we need to gather the trait implementation for all of the generated base types, and pass it off to subsystem bindgen type generators via the callback blocklisted_type_implements_trait.

This PR introduces the mechanism to be able to parse a bindgen generated file using syn and extract the trait implementation for each individual type. In practice, the future bindings generation pipeline will generate the types for the base layer of the WDK, parse it via TraitsMap::from_file, and then insert the created TraitsMap into each subsystem type bindgen via BaseTraitsCallback.

Changes

  • Cargo.toml
    • Bump bindgen from 0.71.0 to 0.72.1
  • crates/wdk-build/Cargo.toml
    • Add syn dependency with the parsing feature enabled
  • crates/wdk-build/src/lib.rs
    • Declare new pub mod Traits
  • crates/wdk-build/src/bindgen.rs
    • Pin size_t_is_usize(true) on wdk_default.
  • crates/wdk-build/src/traits.rs
    • TraitsError: typed error enum for failures.
    • TraitsSet: a struct of bools tracking the five traits bindgen tracks (Copy, Clone, Debug, Default, PartialEq).
    • TraitsSource: enum distinguishing whether a type's traits are immediately known (from a #[derive()] attribute or from a base type) or come from an aliased type.
    • BaseTraitsCallback: a bindgen::callbacks::ParseCallbacks implementation that answers blocklisted_type_implements_trait from a populated TraitsMap. Plugs into bindgen's chain so types blocklisted in one bindings module can still be referenced from another. Used in follow up PR.
    • TraitsMap: a map from type name to TraitsSet. Builds incrementally from Rust source files via syn, and resolves alias chains.
    • syn helpers -- extract and parse functions to navigate syn types
    • Tests: unit-test coverage for parsing, alias resolution (including cycle detection and stable iteration order), classification of #[derive(...)] attributes, and syn helpers.
  • crates/wdk-build/tests/traits.rs
  • Integration tests testing full end to end flow

Validation

Tested this with the full implementation of the mutually exclusive header resolution and ensured that all outputted bindings had full #[derive] parity against bindings generated from microsoft/main.

Copilot AI lite review requested due to automatic review settings April 29, 2026 00:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an internal wdk_build::derives module that can parse bindgen-generated Rust (types.rs-style) and reconstruct which auto-derives apply to each emitted type, enabling future multi-pass bindgen runs (e.g., mutually exclusive headers) to preserve derive parity via blocklisted_type_implements_trait.

Changes:

  • Introduces crates/wdk-build/src/derives.rs with DerivesMap, alias resolution, and a ParseCallbacks implementation (BaseDerivesCallback).
  • Updates the default bindgen builder configuration to force size_t/ssize_t to map to usize/isize.
  • Bumps workspace bindgen and adds workspace bitflags, plus syn parsing support in wdk-build.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Cargo.toml Bumps bindgen and adds workspace bitflags dependency.
Cargo.lock Lockfile updates for new/updated dependencies.
crates/wdk-build/Cargo.toml Adds bitflags and syn(parsing) dependencies for derive parsing.
crates/wdk-build/src/lib.rs Exposes internal derives module (doc-hidden).
crates/wdk-build/src/bindgen.rs Sets size_t_is_usize(true) to match Windows driver targets and derive-map assumptions.
crates/wdk-build/src/derives.rs New derive parsing + alias-resolution logic + unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/wdk-build/src/derives.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/wdk-build/Cargo.toml Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Apr 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.78936% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.42%. Comparing base (6b040c0) to head (08fe8e2).

Files with missing lines Patch % Lines
crates/wdk-build/src/traits.rs 94.78% 32 Missing and 15 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #652      +/-   ##
==========================================
+ Coverage   80.47%   82.42%   +1.94%     
==========================================
  Files          26       27       +1     
  Lines        5722     6624     +902     
  Branches     5722     6624     +902     
==========================================
+ Hits         4605     5460     +855     
- Misses        989     1021      +32     
- Partials      128      143      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/tests/derives.rs Outdated
@gurry

Copy link
Copy Markdown
Contributor

Leon Durrenberger (@leon-xd) can parsing base types have any measurable impact on build time?

@wmmc88

Copy link
Copy Markdown
Collaborator

Leon Durrenberger (@leon-xd) can parsing base types have any measurable impact on build time?

Gurinder Singh (@gurry) in the pr description of Leon's other pr which uses this new feature, he says:

I also timed the build process with this change to ensure there was not a significant build time regression. As it stands I am currently able to build wdk_sys with all_features in ~45 seconds, compared to microsoft/main's 60 seconds. I would not trust this number as the build script is subject to change with future revisions for this PR.

So while technically its not attributable to only this change, as a whole the change reduces build time. I think a theory was that bindgen's parsing of header seems to scale non-linearly

@gurry Gurinder Singh (gurry) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Some minor suggestions

Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/tests/traits.rs
Copilot AI review requested due to automatic review settings May 15, 2026 17:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Comment thread crates/wdk-build/src/bindgen.rs
Comment thread crates/wdk-build/Cargo.toml Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
/// X = Y;` target.
const PRIMITIVES: &[&str] = &[
"bool", "char", "f32", "f64", "i8", "i16", "i32", "i64", "i128", "isize", "u8", "u16", "u32",
"u64", "u128", "usize",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: may want to add nightly flagged support for f16/f128: rust-lang/rust#116909. could even add a test that will start to fail once those types are stabilized so we know to update this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added f16 and f128 ahead of time, didn't add test -- should we do that?

Comment thread crates/wdk-build/src/lib.rs Outdated
mod utils;

mod bindgen;
#[doc(hidden)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why is this hidden?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's hidden because this is functionality specifically for wdk-sys. This is not intended to be a public feature but given that it's only for the wdk-sys build script I figured that keeping it out of the docs was enough to keep this from being unused by doc. iirc i looked at how other examples of this was handled in the repo -- should I handle this differently?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if its part of our public api, it should just be public. It's already a public module, and also intended to be used by external crates. The only time where doc(hidden) is really typically used is for providing functions to proc-macros, where there is a language limitation on keeping something like that contained

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed this to be a public mod

Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Comment thread crates/wdk-build/src/derives.rs Outdated
Copilot AI review requested due to automatic review settings July 14, 2026 00:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 14, 2026 00:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

@wmmc88 Melvin Wang (wmmc88) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall shape is good. Just some cleanup needed

Comment thread crates/wdk-build/src/derives.rs Outdated
@@ -0,0 +1,1206 @@
// Copyright (c) Microsoft Corporation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This file has multiple references to a "parser". To be clear, this is just referring to DerivesMap::from_file (since there's no actual parser object or module)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed language of "parser" -- that was from an old revision.

Comment thread crates/wdk-build/src/derives.rs Outdated

impl DerivesMap {
/// Reads a Rust source file from disk and parses its derive
/// information. See [`DerivesMap::from_source`] for the parsing behavior.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

DerivesMap::from_source is a private method, so I would assume this rustdoc linking should fail. Is it passing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed rustdoc linking

Comment thread crates/wdk-build/src/derives.rs Outdated
/// Returns whether `name`'s recorded derive set contains `derive_trait`.
/// Returns `false` if `name` is not recorded.
#[must_use]
pub fn satisfies(&self, name: &str, derive_trait: DeriveTrait) -> bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. Does this actually need to be public? I only see this used internally and not in feat: separate bindgen runs to enable inclusion of mutually-exclusive headers #654. I'm also trying to be mindful of new extra public api's were exposing (since they carry back-compat guarantees that can needlessly bump versions)

  2. nit: naming is awkward. map.satisfies('MyStruct', DeriveTrait::Copy) makes it seem like the map satisfies 'MyStruct' or is otherwise confusing. I think this is mostly a smell that leads me to point 3.

  3. Is there a reason to not just expose the underlying types HashMap? Or implement the same traits and accessor apis from HashMap on the DerivesMap type (ex. https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.get, https://doc.rust-lang.org/std/collections/struct.HashMap.html#impl-Index%3C%26Q%3E-for-HashMap%3CK,+V,+S,+A%3E)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Another point on 3 is that DerivesMap's api doesn't actually act as a map-like type right now- it just contains a map-like type but it doesn't have the APIs to act like a map itself

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Based method implementation on std::collections::HashSet including implementing the Index trait

Comment thread crates/wdk-build/src/derives.rs Outdated

/// Map a bindgen `#[derive(...)]` ident to the tracked [`DeriveTrait`] it
/// represents, or `None` if the parser does not track it.
fn derive_trait_from_name(name: &str) -> Option<DeriveTrait> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there any reason this isn't just a TryFrom on DeriveTrait?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Orphan rule. But this did end up being a bad smell -- construction is now a TryFrom with two different impls: a vec of Strings and a single String

Comment thread crates/wdk-build/src/derives.rs Outdated
Ok(derives_map)
}

fn with_std_types() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Constructors are typically public

@leon-xd Leon Durrenberger (leon-xd) Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was only used internally and we want this to not be an access point -- in my ideal world the only way to construct a TraitMap would be through pointing it at a Rust file (e.g. from_file, which is currently pub). But because it was only used once I just removed it and moved the logic to the callsite (TraitMap::from_source)

Comment thread crates/wdk-build/src/derives.rs Outdated
}

/// Collects the derive trait names from a `#[derive(...)]` attribute list.
fn derives_from_attrs(attrs: &[Attribute]) -> Vec<String> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed function names

Comment thread crates/wdk-build/src/derives.rs Outdated
path.segments
.into_iter()
.next_back()
.map(|seg| seg.ident.to_string())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Its worth calling out that this will break for custom derives or derives with ambiguous trailing path segments, but that it doesn't matter since it later gets discarded

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this got changed/outdated -- specify a bit more that this is only collecting tracked trait names. before this was returning a string I believe but we are now doing the conversion into TraitsSet in this method which makes it a bit clearer overall

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(see current extract_derived_traits_from_attrs for details)

Comment thread crates/wdk-build/src/derives.rs Outdated
let target_derive_set = *self
.types
.get(curr)
.expect("`self.types.contains_key(curr)` just returned true");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed expect messages to be recommended style

Comment thread crates/wdk-build/src/derives.rs Outdated
/// - [`DerivesError::UnsupportedSynNode`] if `ty` is a `syn::Type` variant
/// other than Ptr/Path/Array, or if the path has generic arguments
/// - [`DerivesError::MalformedShape`] if the path has no segments
fn derives_for_type(ty: &Type) -> Result<DerivesSource, DerivesError> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This function has a bunch of implementation details whose intent should be more explicitly documented (i.e. why each set of derives is being added for each case)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

Comment thread crates/wdk-build/src/derives.rs Outdated
}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please organize tests so that each function being tested is a sub module, and each test is a named scenario expectation.

toy example:

fn add(a: u32, b: u32) -> u32 { ... }

#[cfg(test)]
mod tests {
    mod add {
        #[test]
        fn two_negative_values_return_correct_sum() { ... }
    }
}

This makes it clear for when the test harness prints out test names with full paths. For example, tests::add::two_negative_values_return_correct_sum ... FAIL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

organized all tests

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 21 changed files in this pull request and generated 1 comment.

Comment thread crates/wdk-build/src/derives.rs Outdated
Comment on lines +105 to +109
#[allow(
clippy::struct_excessive_bools,
reason = "type represents an independent set of flags, not a state machine"
)]
struct DerivesSet {
Copilot AI review requested due to automatic review settings August 6, 2026 19:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings August 6, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings August 7, 2026 00:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 21 changed files in this pull request and generated 2 comments.

Suppressed comments (8)

crates/wdk-build/src/lib.rs:27

  • The PR title/description focus on wdk_build::DerivesMap / a derives module, but the code introduces wdk_build::traits / TraitsMap instead. Please align the PR metadata with the implemented API (rename module/types or update the title/description). If this module is intended for internal build-script use (as described), consider adding #[doc(hidden)] pub mod traits; to match the stated intent while keeping it public for integration tests.
pub mod traits;

crates/wdk-build/src/traits.rs:385

  • walked.contains(&next) makes cycle detection O(n²) over long alias chains (and bindgen outputs can have many aliases). Consider tracking visited names in a HashSet for O(1) membership checks, while still keeping a Vec to preserve walk order for the error payload.
    fn resolve_type_aliases(
        &mut self,
        type_aliases: &HashMap<String, String>,
    ) -> Result<(), TraitsError> {
        for key in type_aliases.keys() {
            if self.types.contains_key(key) {
                continue;
            }

            let mut curr = key;
            let mut walked = vec![curr];
            while !self.types.contains_key(curr) {
                let Some(next) = type_aliases.get(curr) else {
                    return Err(TraitsError::UnresolvedTypeAlias {
                        target: curr.clone(),
                    });
                };
                if walked.contains(&next) {
                    return Err(TraitsError::TypeAliasCycle {
                        names: walked.into_iter().cloned().collect(),
                    });
                }
                walked.push(next);
                curr = next;
            }

crates/wdk-build/src/traits.rs:194

  • Several doc comments refer to incorrect/nonexistent names and contain typos: TraitsErrror::UntrackedTraits (extra 'r'), TraitsError::UntrackedTrait (variant is UntrackedTraits), and UnsupportedSynNodeVariant (variant is UnsupportedNodeVariant). Also inferedinferred. Please correct these to match the actual error enum/variants and improve accuracy for API consumers.
    /// Returns [`TraitsErrror::UntrackedTraits`] if all traits are not tracked.

crates/wdk-build/src/traits.rs:219

  • Several doc comments refer to incorrect/nonexistent names and contain typos: TraitsErrror::UntrackedTraits (extra 'r'), TraitsError::UntrackedTrait (variant is UntrackedTraits), and UnsupportedSynNodeVariant (variant is UnsupportedNodeVariant). Also inferedinferred. Please correct these to match the actual error enum/variants and improve accuracy for API consumers.
    /// Returns [`TraitsError::UntrackedTrait`] if string does not correspond to

crates/wdk-build/src/traits.rs:311

  • Several doc comments refer to incorrect/nonexistent names and contain typos: TraitsErrror::UntrackedTraits (extra 'r'), TraitsError::UntrackedTrait (variant is UntrackedTraits), and UnsupportedSynNodeVariant (variant is UnsupportedNodeVariant). Also inferedinferred. Please correct these to match the actual error enum/variants and improve accuracy for API consumers.
    /// - [`TraitsError::UnsupportedSynNodeVariant`] or

crates/wdk-build/src/traits.rs:435

  • Several doc comments refer to incorrect/nonexistent names and contain typos: TraitsErrror::UntrackedTraits (extra 'r'), TraitsError::UntrackedTrait (variant is UntrackedTraits), and UnsupportedSynNodeVariant (variant is UnsupportedNodeVariant). Also inferedinferred. Please correct these to match the actual error enum/variants and improve accuracy for API consumers.
/// either be infered if a base type is given or stored as a type alias for

crates/wdk-build/src/traits.rs:435

  • Correct typo in comment: 'infered' should be 'inferred'.
/// either be infered if a base type is given or stored as a type alias for

crates/wdk-build/src/traits.rs:962

  • Minor: return true; at the end of a function is redundant. Returning true directly improves readability and matches idiomatic Rust.
    trace!("Path is recognized as a core_ffi_type");
    return true;
}

/// - [`TraitsError::UnresolvedTypeAlias`] or
/// [`TraitsError::TypeAliasCycle`] if a type alias cannot be resolved to
/// a recorded type
#[tracing::instrument(level = "trace", err(level = Level::TRACE ))]
Comment on lines +249 to +261
impl ParseCallbacks for BaseTraitsCallback {
fn blocklisted_type_implements_trait(
&self,
name: &str,
derive_trait: DeriveTrait,
) -> Option<ImplementsTrait> {
Some(if self.map[name].contains(derive_trait) {
ImplementsTrait::Yes
} else {
ImplementsTrait::No
})
}
}
Comment on lines +632 to +635
Type::Array(arr) => {
trace!("Type recognized as a `Array`");
extract_traits_from_type(&arr.elem)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure how realistic this situation would be but would this allow TraitsSet::default = true for array definitions with length > 32?

The docs mention that trait implementations are generated up to size 32. https://doc.rust-lang.org/std/primitive.array.html

And it looks like Melvin Wang (@wmmc88) ran into issues b/c of it: rust-lang/rust-bindgen#2803

@Alan632 Alan632 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great changes and documentation!

Copilot AI review requested due to automatic review settings August 10, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

crates/wdk-build/src/traits.rs:274

  • BaseTraitsCallback::blocklisted_type_implements_trait indexes into the map with self.map[name], which will panic if bindgen queries a blocklisted type that wasn’t present in the parsed base file (e.g., blocklisted headers may contain types not emitted/allowlisted in the base run). This would turn a missing entry into a build-script crash. Prefer a fallible lookup and return None for unknown names so bindgen can fall back to its default behavior.
        Some(if self.map[name].contains(derive_trait) {
            ImplementsTrait::Yes
        } else {
            ImplementsTrait::No
        })

crates/wdk-build/tests/traits.rs:71

  • The integration-test snippet uses UCHAR = ::core::ffi::c_char, but Windows UCHAR is an unsigned char and bindgen typically models this as ::core::ffi::c_uchar (or u8). Using c_char here could mask issues in the core::ffi classification logic.
        pub type UCHAR = ::core::ffi::c_char;

crates/wdk-build/src/traits.rs:328

  • The from_source docs reference TraitsError::UnsupportedSynNodeVariant, but the enum variant is named UnsupportedNodeVariant. As written, this link will be broken/misleading in rustdoc.
    /// - [`TraitsError::Parse`] if `source` is not valid Rust
    /// - [`TraitsError::UnsupportedSynNodeVariant`] or
    ///   [`TraitsError::UnsupportedNodeShape`] if a classified construct does
    ///   not match any recognized bindgen output shape

crates/wdk-build/src/traits.rs:110

  • TraitsError::UntrackedTraits is currently used when no tracked traits are found (see TraitsSet::try_from(Vec<String>)), but the variant docs say traits were encountered that aren’t tracked. Either update the docs/error name to reflect the actual meaning ("no tracked traits found"), or change construction sites to include only the truly untracked trait names when mixed tracked/untracked inputs are present.
    /// Traits were encountered not belonging to the set of traits that we
    /// track.
    #[error("traits not tracked: {trait_names:?}")]
    UntrackedTraits {
        /// The untracked trait name.
        trait_names: Vec<String>,

crates/wdk-build/src/lib.rs:28

  • The PR description says the new parsing module is intended for internal build-script use and is gated with #[doc(hidden)] under a derives module / DerivesMap API. In this diff it’s exposed as pub mod traits; (public, not doc-hidden) and the types are named TraitsMap/TraitsSet. Either update the PR description/title to match the shipped API naming, or apply the intended #[doc(hidden)] gating here to avoid accidentally committing to a public API surface.
pub mod cargo_make;
pub mod metadata;
pub mod traits;

Comment thread crates/wdk-build/src/traits.rs
Copilot AI review requested due to automatic review settings August 10, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (6)

crates/wdk-build/src/traits.rs:274

  • BaseTraitsCallback::blocklisted_type_implements_trait indexes self.map[name], which panics if the type name is missing from the map. Since the callback API allows returning None for unknown types, this should avoid panicking and return None when name isn’t present.
        Some(if self.map[name].contains(derive_trait) {
            ImplementsTrait::Yes
        } else {
            ImplementsTrait::No
        })

crates/wdk-build/src/traits.rs:1541

  • This test claims str implements all traits except Copy and Default, but the expected TraitsSet only disables copy. Update the expectation to also disable default (matching the actual trait bounds for Default).
        fn str_implements_all_except_copy_and_default() {
            let expected = TraitsSet {
                copy: false,
                ..TraitsSet::all()
            };

crates/wdk-build/src/traits.rs:964

  • The str primitive is treated as “all traits except Copy”, but str cannot implement Default because Default requires Sized. This branch should also clear Default (and the corresponding unit test below should match).

This issue also appears on line 1537 of the same file.

            "Path is recognized as a primitive in the `PRIMITIVES_DERIVE_ALL_EXCEPT_COPY` array"
        );
        let mut set = TraitsSet::all();
        set.remove(DeriveTrait::Copy);

        return Some(set);

crates/wdk-build/src/traits.rs:327

  • Rustdoc references TraitsError::UnsupportedSynNodeVariant, but the enum variant is named UnsupportedNodeVariant. This breaks the intra-doc link and is misleading.
    /// - [`TraitsError::Parse`] if `source` is not valid Rust
    /// - [`TraitsError::UnsupportedSynNodeVariant`] or
    ///   [`TraitsError::UnsupportedNodeShape`] if a classified construct does
    ///   not match any recognized bindgen output shape

crates/wdk-build/src/traits.rs:576

  • Comment grammar: “attributes that are have ident” → “attributes that have ident”.
        .iter()
        // gather only attributes that are have ident "derive"
        .filter(|attr| attr.path().is_ident("derive"))

crates/wdk-build/src/lib.rs:27

  • The PR description says this module is intended for internal build-script use and should be gated with #[doc(hidden)], but it’s currently exported as a normal public module (pub mod traits;). Consider adding #[doc(hidden)] here to match the stated intent and avoid missing_docs warnings for a public module with no docs.
pub mod cargo_make;
pub mod metadata;
pub mod traits;

Copilot AI review requested due to automatic review settings August 10, 2026 22:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (8)

crates/wdk-build/src/traits.rs:1987

  • These assertions are checking the core::ffi type list against PRIMITIVES_DERIVE_ALL, which will fail (e.g., c_char is not a Rust primitive) and doesn’t validate the intended constant set. Use FFI_DERIVE_ALL here.
            for s in all {
                assert!(PRIMITIVES_DERIVE_ALL.contains(s));
            }

crates/wdk-build/src/traits.rs:1990

  • This loop checks c_float/c_double membership in PRIMITIVES_DERIVE_ALL_EXCEPT_HASH, but these are core::ffi types. The test should assert membership in FFI_DERIVE_ALL_EXCEPT_HASH.
            for s in all_except_hash {
                assert!(PRIMITIVES_DERIVE_ALL_EXCEPT_HASH.contains(s));
            }

crates/wdk-build/src/traits.rs:964

  • PRIMITIVES_DERIVE_ALL_EXCEPT_COPY is used for str, but str also does not implement Default. parse_path_for_primitive_traits currently only removes Copy, which makes the returned trait set incorrect for str.
        let mut set = TraitsSet::all();
        set.remove(DeriveTrait::Copy);

        return Some(set);

crates/wdk-build/src/traits.rs:1541

  • This test name/doc says str is missing both Copy and Default, but expected only disables Copy. With the intended semantics, default should be false here too.
            let expected = TraitsSet {
                copy: false,
                ..TraitsSet::all()
            };

crates/wdk-build/src/traits.rs:274

  • blocklisted_type_implements_trait indexes self.map[name], which will panic if bindgen queries a blocklisted type not present in the map (e.g., due to naming differences or an incomplete seed set). For a build-script callback, it’s safer to return None (let bindgen fall back) when the key is absent.
    fn blocklisted_type_implements_trait(
        &self,
        name: &str,
        derive_trait: DeriveTrait,
    ) -> Option<ImplementsTrait> {
        Some(if self.map[name].contains(derive_trait) {
            ImplementsTrait::Yes
        } else {
            ImplementsTrait::No
        })

crates/wdk-build/src/traits.rs:2253

  • This test currently asserts that unknown keys panic, but that’s only true because the callback uses Index internally. If the callback is adjusted to return None for unknown keys (safer for build scripts), update the test accordingly.
        #[test]
        #[should_panic(expected = "accessing type name that doesn't exist")]
        fn unknown_key_panics() {

crates/wdk-build/src/lib.rs:27

  • The PR description says this parsing module is intended for internal wdk-sys build-script use and is #[doc(hidden)], but it’s currently exported as a normal public module. If it’s not meant to be part of the stable public API surface, consider hiding it from docs (or otherwise clarifying the intended public API).
pub mod cargo_make;
pub mod metadata;
pub mod traits;

crates/wdk-build/src/traits.rs:326

  • Doc link refers to a non-existent TraitsError::UnsupportedSynNodeVariant. The actual enum variant is UnsupportedNodeVariant, so the rustdoc link is currently broken.

This issue also appears in the following locations of the same file:

  • line 961
  • line 1538
  • line 1985
  • line 1988
    /// - [`TraitsError::UnsupportedSynNodeVariant`] or

@leon-xd Leon Durrenberger (leon-xd) changed the title feat: wdk_build::DerivesMap for derives collection of bindgen output feat: wdk_build::TraitsMap for collection of implemented traits for bindgen output Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants