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.

Sign up for GitHub

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 FieldInfo::field_type_name #2863

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Add FieldInfo::field_type_name #2863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d18faee
Add FieldInfo::field_type_name
workingjubilee Jul 5, 2024
c973dc9
Expose the name of the inner type of an alias
pvdrz Dec 2, 2024
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@
- Add option to use DST structs for flexible arrays (--flexarray-dst, #2772).
- Add option to dynamically load variables (#2812).
- Add option in CLI to use rustified non-exhaustive enums (--rustified-non-exhaustive-enum, #2847).
- Add field_type_name to FieldInfo.
## Changed
- Remove which and lazy-static dependencies (#2809, #2817).
- Generate compile-time layout tests (#2787).
Expand Down
2 changes: 2 additions & 0 deletions bindgen/callbacks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,4 +280,6 @@ pub struct FieldInfo<'a> {
pub type_name: &'a str,
/// The name of the field.
pub field_name: &'a str,
/// The name of the type of the field.
pub field_type_name: Option<&'a str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

IT seems we should probably include c++ namespaces and so on here? So, use the same name as all the blocklists etc use.

Copy link
Member Author

Choose a reason for hiding this comment

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

uhhh can I be counseled on how that should be done because I am not very familiar with this codebase, much less C++

Copy link
Member Author

Choose a reason for hiding this comment

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

I also should note that I specifically want None instead of the unnamed at file... output that currently is given in certain other cases, because I think that output is pointless, even harmful: it requires me to filter-by-string for anonymous types, which is much more error-prone and dependent on the vagaries of bindgen's output instead of using a type encoding.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emilio do we include c++ namespaces for the type name as well? otherwise it would be a bit inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pvdrz type_name currently uses canonical_name, so yes, I think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really need the canonical name, though, because I don't need ExprEvalStep__bindgen_ty_1, because what's important to me is that it is an anonymous structure, and I don't necessarily want to reason about anonymous structures by their name. Maybe their other attributes, but the real answer for their name is the empty string.

Copy link
Contributor

@emilio emilio Jul 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, sure, but my point is, if you have:

namespace foo {
  class Bar { ... };
}
namespace baz {
  class Bar { ... };
}

You want to be able to differentiate between one and the other right? btw, it's unclear to me what this does for pointer fields, templates, and so on. I assume the answer is reasonable, but still it'd be worth having some tests.

You might still can make the anonymous / non-anonymous distinction (if thing.name().is_some() { Some(thing.canonical_name()) } else { None } or same using map or what not)

Copy link
Member Author

Choose a reason for hiding this comment

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

My answer is that I don't really care about whatever the answer is to that because I don't use C++, not personally, nor in a work context, and in the one case where I've tried to use bindgen against it in anger, its insistence on qualifying namespaces was actually a disadvantage as the namespaces were effectively meaningless and I would have been better off if the Rust code did not know about them (they were used as a crude implementation of parameterized modules, instead of using something sensible for this facility, like e.g. template parameters).

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I'd rather see this working in C and sort of working in C++ that not seeing it working at all. I think it wouldn't be unreasonable to take this as it is and see if someone else actually ends up using this and requiring the canonical_name as we could either add it as a new field or use it instead of the regular name if a flag is enabled.

@workingjubilee @emilio does that works for both of you?

Copy link
Member Author

@workingjubilee workingjubilee Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure.

And I'd be happy to do the .map-based solution, but my main point of confusion at this juncture is in this code:

rust-bindgen/bindgen/ir/item.rs

Lines 40 to 68 in 2fb25e3

pub(crate) trait ItemCanonicalName {
/// Get the canonical name for this item.
fn canonical_name(&self, ctx: &BindgenContext) -> String;
}
/// The same, but specifies the path that needs to be followed to reach an item.
///
/// To contrast with canonical_name, here's an example:
///
/// ```c++
/// namespace foo {
/// const BAR = 3;
/// }
/// ```
///
/// For bar, the canonical path is `vec!["foo", "BAR"]`, while the canonical
/// name is just `"BAR"`.
pub(crate) trait ItemCanonicalPath {
/// Get the namespace-aware canonical path for this item. This means that if
/// namespaces are disabled, you'll get a single item, and otherwise you get
/// the whole path.
fn namespace_aware_canonical_path(
&self,
ctx: &BindgenContext,
) -> Vec<String>;
/// Get the canonical path for this item.
fn canonical_path(&self, ctx: &BindgenContext) -> Vec<String>;
}

With "namespace-aware canonical paths" being a thing, this suggests it's more complicated than just calling canonical_name(), so I feel, basically, not qualified to evaluate what "canonical name" means in bindgen because it conflates C++ namespaces with generated symbols.

}
6 changes: 6 additions & 0 deletions bindgen/codegen/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,9 @@ impl CodeGenerator for Type {
cb.field_visibility(FieldInfo {
type_name: &item.canonical_name(ctx),
field_name: "0",
field_type_name: inner_item
.expect_type()
.name(),
})
})
.unwrap_or(ctx.options().default_visibility);
Expand Down Expand Up @@ -1544,6 +1547,7 @@ impl FieldCodegen<'_> for FieldData {
cb.field_visibility(FieldInfo {
type_name: &parent_item.canonical_name(ctx),
field_name,
field_type_name: field_ty.name(),
})
}),
self.annotations(),
Expand Down Expand Up @@ -1949,6 +1953,7 @@ impl<'a> FieldCodegen<'a> for Bitfield {

let bitfield_ty_item = ctx.resolve_item(self.ty());
let bitfield_ty = bitfield_ty_item.expect_type();
let bitfield_ty_ident = bitfield_ty.name();

let bitfield_ty_layout = bitfield_ty
.layout(ctx)
Expand All @@ -1973,6 +1978,7 @@ impl<'a> FieldCodegen<'a> for Bitfield {
cb.field_visibility(FieldInfo {
type_name: &parent_item.canonical_name(ctx),
field_name,
field_type_name: bitfield_ty_ident,
})
})
});
Expand Down
Loading