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 unsafe trait support (continued) #155

Merged
merged 11 commits into from
Nov 12, 2023
40 changes: 30 additions & 10 deletions crates/formality-check/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ use anyhow::bail;

use fn_error_context::context;
use formality_core::Downcasted;
use formality_prove::Env;
use formality_prove::{Env, Safety};
use formality_rust::{
grammar::{
AssociatedTy, AssociatedTyBoundData, AssociatedTyValue, AssociatedTyValueBoundData, Fn,
FnBoundData, ImplItem, NegTraitImpl, NegTraitImplBoundData, TraitBoundData, TraitImpl,
TraitImplBoundData, TraitItem,
FnBoundData, ImplItem, NegTraitImpl, NegTraitImplBoundData, Trait, TraitBoundData,
TraitImpl, TraitImplBoundData, TraitItem,
},
prove::ToWcs,
};
Expand All @@ -17,10 +17,8 @@ use formality_types::{
};

impl super::Check<'_> {
#[context("check_trait_impl({v:?})")]
pub(super) fn check_trait_impl(&self, v: &TraitImpl) -> Fallible<()> {
let TraitImpl { binder } = v;

#[context("check_trait_impl({trait_impl:?})")]
pub(super) fn check_trait_impl(&self, trait_impl: &TraitImpl) -> Fallible<()> {
let mut env = Env::default();

let TraitImplBoundData {
Expand All @@ -29,7 +27,7 @@ impl super::Check<'_> {
trait_parameters,
where_clauses,
impl_items,
} = env.instantiate_universally(binder);
} = env.instantiate_universally(&trait_impl.binder);

let trait_ref = trait_id.with(self_ty, trait_parameters);

Expand All @@ -45,32 +43,54 @@ impl super::Check<'_> {
trait_items,
} = trait_decl.binder.instantiate_with(&trait_ref.parameters)?;

self.check_safety_matches(&trait_decl, &trait_impl)?;

for impl_item in &impl_items {
self.check_trait_impl_item(&env, &where_clauses, &trait_items, impl_item)?;
}

Ok(())
}

pub(super) fn check_neg_trait_impl(&self, i: &NegTraitImpl) -> Fallible<()> {
#[context("check_neg_trait_impl({trait_impl:?})")]
pub(super) fn check_neg_trait_impl(&self, trait_impl: &NegTraitImpl) -> Fallible<()> {
let mut env = Env::default();

let NegTraitImplBoundData {
trait_id,
self_ty,
trait_parameters,
where_clauses,
} = env.instantiate_universally(&i.binder);
} = env.instantiate_universally(&trait_impl.binder);

let trait_ref = trait_id.with(self_ty, trait_parameters);

// Negative impls are always safe (rustc E0198) regardless of the trait's safety.
if trait_impl.safety == Safety::Unsafe {
bail!("negative impls cannot be unsafe");
}

self.prove_where_clauses_well_formed(&env, &where_clauses, &where_clauses)?;

self.prove_goal(&env, &where_clauses, trait_ref.not_implemented())?;

Ok(())
}

/// Validate that the declared safety of an impl matches the one from the trait declaration.
fn check_safety_matches(&self, trait_decl: &Trait, trait_impl: &TraitImpl) -> Fallible<()> {
Copy link
Contributor

@nikomatsakis nikomatsakis Nov 12, 2023

Choose a reason for hiding this comment

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

...and I like this version of the fn better

if trait_decl.safety != trait_impl.safety {
match trait_decl.safety {
Safety::Safe => bail!("implementing the trait `{:?}` is not unsafe", trait_decl.id),
Safety::Unsafe => bail!(
"the trait `{:?}` requires an `unsafe impl` declaration",
trait_decl.id
),
}
}
Ok(())
}

fn check_trait_impl_item(
&self,
env: &Env,
Expand Down
6 changes: 5 additions & 1 deletion crates/formality-check/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ use formality_types::grammar::Fallible;
impl super::Check<'_> {
#[context("check_trait({:?})", t.id)]
pub(super) fn check_trait(&self, t: &Trait) -> Fallible<()> {
let Trait { id: _, binder } = t;
let Trait {
safety: _,
id: _,
binder,
} = t;
let mut env = Env::default();

let TraitBoundData {
Expand Down
6 changes: 3 additions & 3 deletions crates/formality-core/src/judgment/test_filtered.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#![cfg(test)]

use std::sync::Arc;
use crate::cast_impl;

use crate::cast_impl;
use crate::judgment_fn;

#[derive(Ord, PartialOrd, Eq, PartialEq, Clone, Debug, Hash)]
Expand All @@ -24,14 +24,14 @@ impl Graph {
judgment_fn!(
fn transitive_reachable(g: Arc<Graph>, node: u32) => u32 {
debug(node, g)

(
(graph.successors(a) => b)
(if b % 2 == 0)
--------------------------------------- ("base")
(transitive_reachable(graph, a) => b)
)

(
(transitive_reachable(&graph, a) => b)
(transitive_reachable(&graph, b) => c)
Expand Down
12 changes: 11 additions & 1 deletion crates/formality-macros/src/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,24 @@ fn debug_variant_with_attr(

fn debug_field_with_mode(name: &Ident, mode: &FieldMode) -> TokenStream {
match mode {
FieldMode::Single | FieldMode::Optional => {
FieldMode::Single => {
quote_spanned! { name.span() =>
write!(fmt, "{}", sep)?;
write!(fmt, "{:?}", #name)?;
sep = " ";
}
}

FieldMode::Optional => {
quote_spanned! { name.span() =>
if !::formality_core::util::is_default(#name) {
write!(fmt, "{}", sep)?;
write!(fmt, "{:?}", #name)?;
sep = " ";
}
}
}

FieldMode::Many => {
quote_spanned! { name.span() =>
for e in #name {
Expand Down
1 change: 1 addition & 0 deletions crates/formality-prove/src/db.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

35 changes: 32 additions & 3 deletions crates/formality-prove/src/decls.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::fmt;

use formality_core::{set, Set, Upcast};
use formality_macros::term;
use formality_types::grammar::{
Expand Down Expand Up @@ -104,8 +106,10 @@ impl Decls {

/// An "impl decl" indicates that a trait is implemented for a given set of types.
/// One "impl decl" is created for each impl in the Rust source.
#[term(impl $binder)]
#[term($?safety impl $binder)]
pub struct ImplDecl {
/// The safety this impl declares, which needs to match the implemented trait's safety.
pub safety: Safety,
/// The binder covers the generic variables from the impl
pub binder: Binder<ImplDeclBoundData>,
}
Expand All @@ -122,8 +126,11 @@ pub struct ImplDeclBoundData {

/// A declaration that some trait will *not* be implemented for a type; derived from negative impls
/// like `impl !Foo for Bar`.
#[term(impl $binder)]
#[term($?safety impl $binder)]
pub struct NegImplDecl {
/// The safety this negative impl declares
pub safety: Safety,

/// Binder comes the generics on the impl
pub binder: Binder<NegImplDeclBoundData>,
}
Expand All @@ -135,15 +142,37 @@ pub struct NegImplDeclBoundData {
pub where_clause: Wcs,
}

/// Mark a trait or trait impl as `unsafe`.
#[term]
#[customize(debug)]
#[derive(Default)]
pub enum Safety {
#[default]
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know you could do this, nifty.

Safe,
Unsafe,
}

impl fmt::Debug for Safety {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the default behavior that #[term] would generate?

Copy link
Member Author

@lqd lqd Nov 12, 2023

Choose a reason for hiding this comment

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

Oh I didn't know this, or test that it would. I'll try, and open a PR to clean this up.

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've fixed this in #156

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Safety::Safe => write!(f, "safe"),
Safety::Unsafe => write!(f, "unsafe"),
}
}
}

/// A "trait declaration" declares a trait that exists, its generics, and its where-clauses.
/// It doesn't capture the trait items, which will be transformed into other sorts of rules.
///
/// In Rust syntax, it covers the `trait Foo: Bar` part of the declaration, but not what appears in the `{...}`.
#[term(trait $id $binder)]
#[term($?safety trait $id $binder)]
pub struct TraitDecl {
/// The name of the trait
pub id: TraitId,

/// Whether the trait is `unsafe` or not
pub safety: Safety,

/// The binder here captures the generics of the trait; it always begins with a `Self` type.
pub binder: Binder<TraitDeclBoundData>,
}
Expand Down
2 changes: 1 addition & 1 deletion crates/formality-prove/src/prove/minimize/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ fn minimize_a() {
let (mut env_min, term_min, m) = minimize(env, term);

expect!["(Env { variables: [?ty_0, ?ty_1], coherence_mode: false }, [?ty_0, ?ty_1])"]
.assert_eq(&format!("{:?}", (&env_min, &term_min)));
.assert_eq(&format!("{:?}", (&env_min, &term_min)));

let ty0 = term_min[0].as_variable().unwrap();
let ty1 = term_min[1].as_variable().unwrap();
Expand Down
10 changes: 7 additions & 3 deletions crates/formality-rust/src/grammar.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::sync::Arc;

use formality_core::{term, Upcast};
use formality_prove::Safety;
use formality_types::{
grammar::{
AdtId, AliasTy, AssociatedItemId, Binder, Const, CrateId, Fallible, FieldId, FnId, Lt,
Expand Down Expand Up @@ -160,8 +161,9 @@ pub struct Variant {
pub fields: Vec<Field>,
}

#[term(trait $id $binder)]
#[term($?safety trait $id $binder)]
pub struct Trait {
pub safety: Safety,
pub id: TraitId,
pub binder: TraitBinder<TraitBoundData>,
}
Expand Down Expand Up @@ -241,8 +243,9 @@ pub struct AssociatedTyBoundData {
pub where_clauses: Vec<WhereClause>,
}

#[term(impl $binder)]
#[term($?safety impl $binder)]
pub struct TraitImpl {
pub safety: Safety,
pub binder: Binder<TraitImplBoundData>,
}

Expand All @@ -267,8 +270,9 @@ impl TraitImplBoundData {
}
}

#[term(impl $binder)]
#[term($?safety impl $binder)]
pub struct NegTraitImpl {
pub safety: Safety,
pub binder: Binder<NegTraitImplBoundData>,
}

Expand Down
12 changes: 8 additions & 4 deletions crates/formality-rust/src/prove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl Crate {
self.items
.iter()
.flat_map(|item| match item {
CrateItem::Trait(Trait { id, binder }) => {
CrateItem::Trait(Trait { id, binder, safety }) => {
let (
vars,
TraitBoundData {
Expand All @@ -89,6 +89,7 @@ impl Crate {
},
) = binder.open();
Some(prove::TraitDecl {
safety: safety.clone(),
id: id.clone(),
binder: Binder::new(
vars,
Expand All @@ -110,7 +111,7 @@ impl Crate {
self.items
.iter()
.flat_map(|item| match item {
CrateItem::TraitImpl(TraitImpl { binder }) => {
CrateItem::TraitImpl(TraitImpl { binder, safety }) => {
let (
vars,
TraitImplBoundData {
Expand All @@ -122,6 +123,7 @@ impl Crate {
},
) = binder.open();
Some(prove::ImplDecl {
safety: safety.clone(),
binder: Binder::new(
vars,
prove::ImplDeclBoundData {
Expand All @@ -140,7 +142,7 @@ impl Crate {
self.items
.iter()
.flat_map(|item| match item {
CrateItem::NegTraitImpl(NegTraitImpl { binder }) => {
CrateItem::NegTraitImpl(NegTraitImpl { binder, safety }) => {
let (
vars,
NegTraitImplBoundData {
Expand All @@ -151,6 +153,7 @@ impl Crate {
},
) = binder.open();
Some(prove::NegImplDecl {
safety: safety.clone(),
binder: Binder::new(
vars,
prove::NegImplDeclBoundData {
Expand All @@ -169,7 +172,7 @@ impl Crate {
self.items
.iter()
.flat_map(|item| match item {
CrateItem::TraitImpl(TraitImpl { binder }) => {
CrateItem::TraitImpl(TraitImpl { binder, safety: _ }) => {
let (
impl_vars,
TraitImplBoundData {
Expand Down Expand Up @@ -226,6 +229,7 @@ impl Crate {
.iter()
.flat_map(|item| match item {
CrateItem::Trait(Trait {
safety: _,
id: trait_id,
binder,
}) => {
Expand Down
1 change: 1 addition & 0 deletions examples/formality-eg/type_system.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

2 changes: 1 addition & 1 deletion rust-toolchain
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[toolchain]
channel = "nightly-2023-10-08"
components = [ "rustc-dev", "llvm-tools" ]
components = ["rustc-dev", "llvm-tools", "rustfmt"]
4 changes: 4 additions & 0 deletions tests/ui/decl_safety/safe_trait-negative_impl_mismatch.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Error: check_neg_trait_impl(unsafe impl ! Foo for u32 {})

Caused by:
negative impls cannot be unsafe
6 changes: 6 additions & 0 deletions tests/ui/decl_safety/safe_trait-negative_impl_mismatch.🔬
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[
crate baguette {
trait Foo {}
unsafe impl !Foo for u32 {}
}
]
7 changes: 7 additions & 0 deletions tests/ui/decl_safety/safe_trait.🔬
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//@check-pass
[
crate baguette {
safe trait Foo {}
safe impl Foo for u32 {}
}
]
4 changes: 4 additions & 0 deletions tests/ui/decl_safety/safe_trait_mismatch.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Error: check_trait_impl(unsafe impl Foo for u32 { })

Caused by:
implementing the trait `Foo` is not unsafe
6 changes: 6 additions & 0 deletions tests/ui/decl_safety/safe_trait_mismatch.🔬
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[
crate baguette {
trait Foo {}
unsafe impl Foo for u32 {}
}
]
7 changes: 7 additions & 0 deletions tests/ui/decl_safety/unsafe_trait-negative_impl.🔬
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//@check-pass
[
crate baguette {
unsafe trait Foo {}
impl !Foo for u32 {}
}
]
Loading
Loading