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

Added target_os cfg feature to header translator #433

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 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
105 changes: 93 additions & 12 deletions crates/header-translator/src/availability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,87 @@ use clang::{Entity, PlatformAvailability, Version};
use crate::context::Context;

#[derive(Debug, Clone, PartialEq, Default)]
struct Unavailable {
ios: bool,
ios_app_extension: bool,
macos: bool,
macos_app_extension: bool,
maccatalyst: bool,
watchos: bool,
tvos: bool,
pub struct Unavailable {
pub(crate) ios: bool,
pub(crate) ios_app_extension: bool,
pub(crate) macos: bool,
pub(crate) macos_app_extension: bool,
pub(crate) maccatalyst: bool,
pub(crate) watchos: bool,
pub(crate) tvos: bool,
pub(crate) library_unavailablility: Option<Box<Unavailable>>,
}

impl Unavailable {
fn list_unavailable_oses(&self) -> Vec<&str> {
let mut unavailable_oses = Vec::new();
if self.ios
&& !self
.library_unavailablility
.as_ref()
.map(|u| u.ios)
.unwrap_or_else(|| false)
{
unavailable_oses.push("ios");
}
if self.macos
&& !self
.library_unavailablility
.as_ref()
.map(|u| u.macos)
.unwrap_or_else(|| false)
{
unavailable_oses.push("macos");
}
if self.tvos
&& !self
.library_unavailablility
.as_ref()
.map(|u| u.tvos)
.unwrap_or_else(|| false)
{
unavailable_oses.push("tvos");
}
if self.watchos
&& !self
.library_unavailablility
.as_ref()
.map(|u| u.watchos)
.unwrap_or_else(|| false)
{
unavailable_oses.push("watchos");
}
unavailable_oses
}

/// In some cases of enums, we need to know the availability of the parent enum and the enum
/// variant.
pub fn merge(&self, other: &Self) -> Self {
Self {
ios: self.ios || other.ios,
ios_app_extension: self.ios_app_extension || other.ios_app_extension,
macos: self.macos || other.macos,
macos_app_extension: self.macos_app_extension || other.macos_app_extension,
tvos: self.tvos || other.tvos,
watchos: self.watchos || other.watchos,
maccatalyst: self.maccatalyst || other.maccatalyst,
library_unavailablility: self.library_unavailablility.clone(),
}
}
}
impl fmt::Display for Unavailable {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let unavailable_oses = self.list_unavailable_oses();
let unavailable_oses = unavailable_oses
.iter()
.map(|os| format!("target_os = \"{os}\""))
.collect::<Vec<String>>()
.join(",");
if !unavailable_oses.is_empty() {
write!(f, "#[cfg(not(any({unavailable_oses})))]")?;
simlay marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

@madsmtm madsmtm Apr 7, 2023

Choose a reason for hiding this comment

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

I know that the headers expose this as "unavailability", but I think it would look nicer in the docs if it was inverted; e.g. that #[cfg(not(any(target_os = "watchos")))] became #[cfg(any(target_os = "tvos", target_os = "ios", target_os = "macos"))] instead (this is also how Apple's own docs does it).

This interferes with GNUStep of course, but perhaps we can just code in a manual override there for all of AppKit and Foundation for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually spent a little bit of time doing both variants - all(not(target_os = "ios"), any(taget_os = "macos", target_os = "tvos", target_os = "watchos")). I didn't like it as then everything had a cfg attribute.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I agree with that part

}
Ok(())
}
}

#[derive(Debug, Clone, PartialEq, Default)]
Expand All @@ -29,20 +102,27 @@ struct Versions {

#[derive(Debug, Clone, PartialEq)]
pub struct Availability {
Copy link
Owner

Choose a reason for hiding this comment

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

#[deprecated] now also appears on impl ClassType, which is incorrect.

So perhaps we should split out the Unavailable from Availability (and maybe rename it to PlatformCfg)? Since that part is always going to be a cfg-gate, which has interactions with higher-level cfg-gates (and must propagate to imports like generated/[Framework]/mod.rs), while introduced and deprecated will always only need to apply to the statement/method/field they're associated with.

Maybe it would even make sense to add PlatformCfg as a field of ItemIdentifier? Since these two are access so often together.

unavailable: Unavailable,
pub(crate) unavailable: Unavailable,
introduced: Versions,
deprecated: Versions,
message: Option<String>,
_swift: Option<PlatformAvailability>,
}

impl Availability {
pub fn parse(entity: &Entity<'_>, _context: &Context<'_>) -> Self {
pub fn parse(
entity: &Entity<'_>,
_context: &Context<'_>,
library_unavailablility: &Unavailable,
) -> Self {
let availabilities = entity
.get_platform_availability()
.expect("platform availability");

let mut unavailable = Unavailable::default();
let mut unavailable = Unavailable {
library_unavailablility: Some(Box::new(library_unavailablility.clone())),
..Default::default()
};
let mut introduced = Versions::default();
let mut deprecated = Versions::default();
let mut message = None;
Expand Down Expand Up @@ -157,7 +237,8 @@ impl fmt::Display for Availability {
}
}
}
// TODO: Emit `cfg` attributes based on `self.unavailable`
write!(f, "{}", self.unavailable)?;

// TODO: Emit availability checks based on `self.introduced`
Ok(())
}
Expand Down
3 changes: 2 additions & 1 deletion crates/header-translator/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ impl<'a> Cache<'a> {
id,
generics,
superclasses,
availability,
..
} => {
let _span = debug_span!("Stmt::ClassDecl", ?id).entered();
Expand Down Expand Up @@ -203,7 +204,7 @@ impl<'a> Cache<'a> {
cls: id.clone(),
generics: generics.clone(),
category: cache.category.clone(),
availability: cache.availability.clone(),
availability: availability.clone(),
superclasses: superclasses.clone(),
methods,
description: Some(format!(
Expand Down
13 changes: 13 additions & 0 deletions crates/header-translator/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::path::Path;

use serde::Deserialize;

use crate::availability::Unavailable;
use crate::data;
use crate::rust_type::Ownership;
use crate::stmt::Derives;
Expand Down Expand Up @@ -81,6 +82,18 @@ pub struct LibraryData {
#[serde(default)]
pub watchos: Option<semver::VersionReq>,
}
impl LibraryData {
pub(crate) fn unavailability(&self) -> Unavailable {
Unavailable {
ios: self.ios.is_none(),
macos: self.macos.is_none(),
tvos: self.tvos.is_none(),
watchos: self.watchos.is_none(),
maccatalyst: self.maccatalyst.is_none(),
..Default::default()
}
}
}

#[derive(Deserialize, Debug, Default, Clone, PartialEq, Eq)]
#[serde(deny_unknown_fields)]
Expand Down
33 changes: 16 additions & 17 deletions crates/header-translator/src/library.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@ use std::fs;
use std::io;
use std::path::Path;

use crate::availability::Unavailable;
use crate::file::{File, FILE_PRELUDE};

#[derive(Debug, PartialEq, Default)]
pub struct Library {
pub files: BTreeMap<String, File>,
pub unavailability: Unavailable,
}

impl Library {
pub fn new() -> Self {
pub(crate) fn new(unavailability: Unavailable) -> Self {
Self {
files: BTreeMap::new(),
unavailability,
}
}

Expand Down Expand Up @@ -60,16 +63,16 @@ impl fmt::Display for Library {
// NOTE: some SDK files have '+' in the file name
let name = name.replace('+', "_");
for stmt in &file.stmts {
let mut iter = stmt.declared_types();
if let Some(item) = iter.next() {
// Use a set to deduplicate features, and to have them in
// a consistent order
let mut features = BTreeSet::new();
stmt.visit_required_types(|item| {
if let Some(feature) = item.feature() {
features.insert(format!("feature = \"{feature}\""));
}
});
// Use a set to deduplicate features, and to have them in
// a consistent order
let mut features = BTreeSet::new();
stmt.visit_required_types(|item| {
if let Some(feature) = item.feature() {
features.insert(format!("feature = \"{feature}\""));
}
});

for (item, unavailability) in stmt.declared_types() {
madsmtm marked this conversation as resolved.
Show resolved Hide resolved
match features.len() {
0 => {}
1 => {
Expand All @@ -87,12 +90,8 @@ impl fmt::Display for Library {
)?;
}
}

writeln!(f, "pub use self::__{name}::{{{item}")?;
for item in iter {
writeln!(f, ", {item}")?;
}
writeln!(f, "}};")?;
write!(f, "{unavailability}")?;
writeln!(f, "pub use self::__{name}::{{{item}}};")?;
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/header-translator/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ fn parse_sdk(index: &Index<'_>, sdk: &SdkPath, llvm_target: &str, config: &Confi
let tu = get_translation_unit(index, sdk, llvm_target);

let mut preprocessing = true;
let mut result = Output::from_libraries(config.libraries.keys());
let mut result = Output::from_libraries(&config.libraries);

let mut library_span = None;
let mut library_span_name = String::new();
Expand Down Expand Up @@ -269,7 +269,7 @@ fn parse_sdk(index: &Index<'_>, sdk: &SdkPath, llvm_target: &str, config: &Confi
preprocessing = false;
// No more includes / macro expansions after this line
let file = library.files.get_mut(&file_name).expect("file");
for stmt in Stmt::parse(&entity, &context) {
for stmt in Stmt::parse(&entity, &context, &library.unavailability) {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of passing the (un-)availability around everywhere like this, I think it would be fine to introduce a few optional fields in Context<'_> that carried this information (since the context is already passed everywhere).

This would also be useful for other things in the future.

So something like (you can freely add only the stuff you need now, just expanding on the example to show the idea):

struct Context<'a> {
    // ... Existing fields

    /// Set whenever we know what the current library is.
    current_library: Cell<Option<Arc<Library>>>,
    /// Set when parsing methods, struct fields, enum fields, ...
    current_type: Cell<Option<(ItemIdentifier, Unavailability)>>,
}

impl Context<'_> {
    fn library(&self) -> Arc<Library> {
        self.current_library.clone().take()
    }

    fn in_library<R>(&self, library: Arc<Library>, f: impl FnOnce(&Self) -> R) -> R {
        self.current_library.set(Some(library));
        let result = f(self);
        self.current_library.set(None);
        result
    }

    // ... Similar methods for the others
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did part of this by adding current_unavailability to Context. In this specific case library is a &mut Library and so passing it in as even a Arc<Library> would require a clone (I think?). Library doesn't derive(Clone)and I didn't feel like adding that. It's definitely cleaner but I'm not in love with it.

file.add_stmt(stmt);
}
}
Expand Down
8 changes: 5 additions & 3 deletions crates/header-translator/src/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::fmt;
use clang::{Entity, EntityKind, ObjCAttributes, ObjCQualifiers};
use tracing::span::EnteredSpan;

use crate::availability::Availability;
use crate::availability::{Availability, Unavailable};
use crate::config::MethodData;
use crate::context::Context;
use crate::id::ItemIdentifier;
Expand Down Expand Up @@ -328,6 +328,7 @@ impl<'tu> PartialMethod<'tu> {
data: MethodData,
is_protocol: bool,
context: &Context<'_>,
library_unavailablility: &Unavailable,
) -> Option<(bool, Method)> {
let Self {
entity,
Expand All @@ -346,7 +347,7 @@ impl<'tu> PartialMethod<'tu> {
return None;
}

let availability = Availability::parse(&entity, context);
let availability = Availability::parse(&entity, context, library_unavailablility);

let modifiers = MethodModifiers::parse(&entity, context);

Expand Down Expand Up @@ -465,6 +466,7 @@ impl PartialProperty<'_> {
setter_data: Option<MethodData>,
is_protocol: bool,
context: &Context<'_>,
library_unavailablility: &Unavailable,
) -> (Option<Method>, Option<Method>) {
let Self {
entity,
Expand All @@ -483,7 +485,7 @@ impl PartialProperty<'_> {
return (None, None);
}

let availability = Availability::parse(&entity, context);
let availability = Availability::parse(&entity, context, library_unavailablility);

let modifiers = MethodModifiers::parse(&entity, context);

Expand Down
9 changes: 5 additions & 4 deletions crates/header-translator/src/output.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::collections::HashMap;
use std::collections::{BTreeMap, BTreeSet};
use std::str::FromStr;

use crate::config::Config;
use crate::config::{Config, LibraryData};
use crate::library::Library;
use crate::stmt::Stmt;

Expand All @@ -11,10 +12,10 @@ pub struct Output {
}

impl Output {
pub fn from_libraries(libraries: impl IntoIterator<Item = impl Into<String>>) -> Self {
pub fn from_libraries(libraries: &HashMap<String, LibraryData>) -> Self {
let libraries = libraries
.into_iter()
.map(|name| (name.into(), Library::new()))
.iter()
.map(|(name, library_data)| (name.into(), Library::new(library_data.unavailability())))
.collect();
Self { libraries }
}
Expand Down
Loading