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

Implement sealed trait analysis and expose it in the adapter. (#343) #345

Merged
merged 1 commit into from
Aug 18, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 6 additions & 1 deletion src/adapter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,12 @@ impl<'a> Adapter<'a> for RustdocAdapter<'a> {
"AttributeMetaItem" => {
properties::resolve_attribute_meta_item_property(contexts, property_name)
}
"Trait" => properties::resolve_trait_property(contexts, property_name),
"Trait" => properties::resolve_trait_property(
contexts,
property_name,
self.current_crate,
self.previous_crate,
),
"ImplementedTrait" => {
properties::resolve_implemented_trait_property(contexts, property_name)
}
Expand Down
17 changes: 15 additions & 2 deletions src/adapter/properties.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ use trustfall::{
FieldValue,
};

use crate::attributes::Attribute;
use crate::{attributes::Attribute, IndexedCrate};

use super::vertex::Vertex;
use super::{origin::Origin, vertex::Vertex};

pub(super) fn resolve_crate_property<'a, V: AsVertex<Vertex<'a>> + 'a>(
contexts: ContextIterator<'a, V>,
Expand Down Expand Up @@ -456,10 +456,23 @@ pub(super) fn resolve_raw_type_property<'a, V: AsVertex<Vertex<'a>> + 'a>(
pub(super) fn resolve_trait_property<'a, V: AsVertex<Vertex<'a>> + 'a>(
contexts: ContextIterator<'a, V>,
property_name: &str,
current_crate: &'a IndexedCrate<'a>,
previous_crate: Option<&'a IndexedCrate<'a>>,
) -> ContextOutcomeIterator<'a, V, FieldValue> {
match property_name {
"unsafe" => resolve_property_with(contexts, field_property!(as_trait, is_unsafe)),
"object_safe" => resolve_property_with(contexts, field_property!(as_trait, is_object_safe)),
"sealed" => resolve_property_with(contexts, move |vertex| {
let trait_item = vertex.as_item().expect("not an Item");
let origin = vertex.origin;

let indexed_crate = match origin {
Origin::CurrentCrate => current_crate,
Origin::PreviousCrate => previous_crate.expect("no previous crate provided"),
};

indexed_crate.is_trait_sealed(&trait_item.id).into()
}),
_ => unreachable!("Trait property {property_name}"),
}
}
Expand Down
122 changes: 122 additions & 0 deletions src/adapter/tests.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
// The Trustfall API requires the adapter to be passed in as an Arc.
// Our adapter is not Send/Sync (it doesn't need it),
// but there's currently nothing we can do about this lint.
#![allow(clippy::arc_with_non_send_sync)]

use std::collections::BTreeMap;
use std::sync::Arc;

Expand Down Expand Up @@ -154,6 +159,123 @@ fn rustdoc_finds_supertrait() {
);
}

#[test]
fn rustdoc_sealed_traits() {
let path = "./localdata/test_data/sealed_traits/rustdoc.json";
let content = std::fs::read_to_string(path)
.with_context(|| format!("Could not load {path} file, did you forget to run ./scripts/regenerate_test_rustdocs.sh ?"))
.expect("failed to load rustdoc");

let crate_ = serde_json::from_str(&content).expect("failed to parse rustdoc");
let indexed_crate = IndexedCrate::new(&crate_);
let adapter = RustdocAdapter::new(&indexed_crate, None);

let query = r#"
{
Crate {
item {
... on Trait {
name @output
sealed @output
}
}
}
}
"#;

let variables: BTreeMap<&str, &str> = BTreeMap::default();

let schema =
Schema::parse(include_str!("../rustdoc_schema.graphql")).expect("schema failed to parse");

#[derive(Debug, PartialOrd, Ord, PartialEq, Eq, serde::Deserialize)]
struct Output {
name: String,
sealed: bool,
}

let mut results: Vec<_> =
trustfall::execute_query(&schema, adapter.into(), query, variables.clone())
.expect("failed to run query")
.map(|row| row.try_into_struct().expect("shape mismatch"))
.collect();
results.sort_unstable();

let mut expected_results = vec![
Output {
name: "Sealed".into(),
sealed: true,
},
Output {
name: "InternalMarker".into(),
sealed: true,
},
Output {
name: "DirectlyTraitSealed".into(),
sealed: true,
},
Output {
name: "TransitivelyTraitSealed".into(),
sealed: true,
},
Output {
name: "Unsealed".into(),
sealed: false,
},
Output {
name: "MethodSealed".into(),
sealed: true,
},
Output {
name: "TransitivelyMethodSealed".into(),
sealed: true,
},
Output {
name: "NotMethodSealedBecauseOfDefaultImpl".into(),
sealed: false,
},
Output {
name: "NotTransitivelySealed".into(),
sealed: false,
},
Output {
name: "GenericSealed".into(),
sealed: true,
},
Output {
name: "NotGenericSealedBecauseOfDefaultImpl".into(),
sealed: false,
},
Output {
name: "IteratorExt".into(),
sealed: false,
},
Output {
name: "Iterator".into(),
sealed: true,
},
Output {
name: "ShadowedSubIterator".into(),
sealed: true,
},
Output {
name: "Super".into(),
sealed: false,
},
Output {
name: "Marker".into(),
sealed: true,
},
Output {
name: "NotGenericSealedBecauseOfPubSupertrait".into(),
sealed: false,
},
];
expected_results.sort_unstable();

similar_asserts::assert_eq!(expected_results, results,);
}

#[test]
fn rustdoc_finds_consts() {
let path = "./localdata/test_data/consts/rustdoc.json";
Expand Down
66 changes: 65 additions & 1 deletion src/indexed_crate.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::{
borrow::Borrow,
cell::RefCell,
collections::{BTreeSet, HashMap},
};

use rustdoc_types::{Crate, Id, Item};

use crate::{adapter::supported_item_kind, visibility_tracker::VisibilityTracker};
use crate::{adapter::supported_item_kind, sealed_trait, visibility_tracker::VisibilityTracker};

/// The rustdoc for a crate, together with associated indexed data to speed up common operations.
///
Expand All @@ -25,6 +26,11 @@ pub struct IndexedCrate<'a> {
/// index: impl owner + impl'd item name -> list of (impl itself, the named item))
pub(crate) impl_index: Option<HashMap<ImplEntry<'a>, Vec<(&'a Item, &'a Item)>>>,

/// Caching whether a trait by the given `Id`` is known to be sealed, or known to be unsealed.
///
/// If the `Id` isn't present in the cache, the trait's sealed status has not yet been computed.
sealed_trait_cache: RefCell<HashMap<&'a Id, bool>>,

/// Trait items defined in external crates are not present in the `inner: &Crate` field,
/// even if they are implemented by a type in that crate. This also includes
/// Rust's built-in traits like `Debug, Send, Eq` etc.
Expand All @@ -49,6 +55,7 @@ impl<'a> IndexedCrate<'a> {
manually_inlined_builtin_traits: create_manually_inlined_builtin_traits(crate_),
imports_index: None,
impl_index: None,
sealed_trait_cache: Default::default(),
};

let mut imports_index: HashMap<Path, Vec<(&Item, Modifiers)>> =
Expand Down Expand Up @@ -153,6 +160,63 @@ impl<'a> IndexedCrate<'a> {
Default::default()
}
}

/// Return `true` if our analysis indicates the trait is sealed, and `false` otherwise.
///
/// Our analysis is conservative: it has false-negatives but no false-positives.
/// If this method returns `true`, the trait is *definitely* sealed or else you've found a bug.
/// It may be possible to construct traits that *technically* are sealed for which our analysis
/// returns `false`.
///
/// The goal of this method is to reflect author intent, not technicalities.
/// When Rustaceans seal traits on purpose, they do so with a limited number of techniques
/// that are well-defined and immediately recognizable to readers in the community:
/// - <https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/>
/// - <https://jack.wrenn.fyi/blog/private-trait-methods/>
///
/// The analysis here looks for such techniques, which are always applied at the type signature
/// level. It does not inspect function bodies or do interprocedural analysis.
///
/// ## Panics
///
/// This method will panic if the provided `id` is not an item in this crate,
/// or does not correspond to a trait in this crate.
///
/// ## Re-entrancy
///
/// This method is re-entrant: calling it may cause additional calls to itself, inquiring about
/// the sealed-ness of a trait's supertraits.
///
/// We rely on rustc to reject supertrait cycles in order to prevent infinite loops.
/// Here's a supertrait cycle that must be rejected by rustc:
/// ```compile_fail
/// pub trait First: Third {}
///
/// pub trait Second: First {}
///
/// pub trait Third: Second {}
/// ```
pub fn is_trait_sealed(&self, id: &'a Id) -> bool {
let cache_ref = self.sealed_trait_cache.borrow();
if let Some(cached) = cache_ref.get(id) {
return *cached;
}

// Explicitly drop the ref, because this method is re-entrant
// and may need to write in a subsequent re-entrant call.
drop(cache_ref);

let trait_item = &self.inner.index[id];
let decision = sealed_trait::is_trait_sealed(self, trait_item);

// Unfortunately there's no easy way to avoid the double-hashing here.
// This method is re-entrant (we may need to decide if a supertrait is sealed *first*),
// and the borrow-checker doesn't like holding the `entry()` API's mut borrow
// while resolving the answer in `.or_insert_with()`.
self.sealed_trait_cache.borrow_mut().insert(id, decision);

decision
}
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
mod adapter;
mod attributes;
mod indexed_crate;
mod sealed_trait;
mod visibility_tracker;

#[cfg(test)]
pub(crate) mod test_util;

mod visibility_tracker;

// Re-export the Crate type so we can deserialize it.
pub use rustdoc_types::Crate;

Expand Down
15 changes: 15 additions & 0 deletions src/rustdoc_schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -902,9 +902,24 @@ type Trait implements Item & Importable {
visibility_limit: String!

# own properties
"""
Whether this trait is defined as `unsafe` and requires the `unsafe` keyword when implementing.
"""
unsafe: Boolean!

"""
Whether this trait can be used as `dyn Trait`.
"""
object_safe: Boolean!

"""
Whether downstream crates are prevented from implementing this trait themselves.

Required implementation items can be added to sealed traits without causing a breaking change,
since no implementations of that trait may exist in other crates.
"""
sealed: Boolean!

# edges from Item
span: Span
attribute: [Attribute!]
Expand Down
Loading