Skip to content

Remove subtyping for () from node graph type system #2418

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Mar 26, 2025
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
16 changes: 0 additions & 16 deletions node-graph/gcore/src/registry.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::transform::Footprint;
use crate::{Node, NodeIO, NodeIOTypes, Type, WasmNotSend};
use dyn_any::{DynAny, StaticType};
use std::collections::HashMap;
Expand Down Expand Up @@ -251,21 +250,6 @@ where
};
match dyn_any::downcast(input) {
Ok(input) => Box::pin(output(*input)),
// If the input type of the node is `()` and we supply an invalid type, we can still call the
// node and just ignore the input and call it with the unit type instead.
Err(_) if core::any::TypeId::of::<_I::Static>() == core::any::TypeId::of::<()>() => {
assert_eq!(std::mem::size_of::<_I>(), 0);
// Rust can't know, that `_I` and `()` are the same size, so we have to use a `transmute_copy()` here
Box::pin(output(unsafe { std::mem::transmute_copy(&()) }))
}
// If the Node expects a footprint but we provide (). In this case construct the default Footprint and pass that
// This is pretty hacky pls fix
Err(_) if core::any::TypeId::of::<_I::Static>() == core::any::TypeId::of::<Footprint>() => {
assert_eq!(std::mem::size_of::<_I>(), std::mem::size_of::<Footprint>());
assert_eq!(std::mem::align_of::<_I>(), std::mem::align_of::<Footprint>());
// Rust can't know, that `_I` and `Footprint` are the same size, so we have to use a `transmute_copy()` here
Box::pin(output(unsafe { std::mem::transmute_copy(&Footprint::default()) }))
}
Err(e) => panic!("DynAnyNode Input, {0} in:\n{1}", e, node_name),
}
}
Expand Down
12 changes: 7 additions & 5 deletions node-graph/graph-craft/src/proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,13 +691,14 @@ impl TypingContext {

/// Checks if a proposed input to a particular (primary or secondary) input connector is valid for its type signature.
/// `from` indicates the value given to a input, `to` indicates the input's allowed type as specified by its type signature.
fn valid_subtype(from: &Type, to: &Type) -> bool {
fn valid_type(from: &Type, to: &Type) -> bool {
match (from, to) {
// Direct comparison of two concrete types.
(Type::Concrete(type1), Type::Concrete(type2)) => type1 == type2,
// Check inner type for futures
(Type::Future(type1), Type::Future(type2)) => type1 == type2,
// Loose comparison of function types, where loose means that functions are considered on a "greater than or equal to" basis of its function type's generality.
// Direct comparison of two function types.
// Note: in the presence of subtyping, functions are considered on a "greater than or equal to" basis of its function type's generality.
// That means we compare their types with a contravariant relationship, which means that a more general type signature may be substituted for a more specific type signature.
// For example, we allow `T -> V` to be substituted with `T' -> V` or `() -> V` where T' and () are more specific than T.
// This allows us to supply anything to a function that is satisfied with `()`.
Expand All @@ -706,8 +707,9 @@ impl TypingContext {
// - `V >= V' ⇒ (T -> V) >= (T -> V')` (functions are covariant in their output types)
// While these two relations aren't a truth about the universe, they are a design decision that we are employing in our language design that is also common in other languages.
// For example, Rust implements these same relations as it describes here: <https://doc.rust-lang.org/nomicon/subtyping.html>
// Graphite doesn't have subtyping currently, but it used to have it, and may do so again, so we make sure to compare types in this way to make things easier.
// More details explained here: <https://github.com/GraphiteEditor/Graphite/issues/1741>
(Type::Fn(in1, out1), Type::Fn(in2, out2)) => valid_subtype(out2, out1) && (valid_subtype(in1, in2) || **in1 == concrete!(())),
(Type::Fn(in1, out1), Type::Fn(in2, out2)) => valid_type(out2, out1) && valid_type(in1, in2),
// If either the proposed input or the allowed input are generic, we allow the substitution (meaning this is a valid subtype).
// TODO: Add proper generic counting which is not based on the name
(Type::Generic(_), _) | (_, Type::Generic(_)) => true,
Expand All @@ -719,7 +721,7 @@ impl TypingContext {
// List of all implementations that match the input types
let valid_output_types = impls
.keys()
.filter(|node_io| valid_subtype(&node_io.call_argument, &primary_input_or_call_argument) && inputs.iter().zip(node_io.inputs.iter()).all(|(p1, p2)| valid_subtype(p1, p2)))
.filter(|node_io| valid_type(&node_io.call_argument, &primary_input_or_call_argument) && inputs.iter().zip(node_io.inputs.iter()).all(|(p1, p2)| valid_type(p1, p2)))
.collect::<Vec<_>>();

// Attempt to substitute generic types with concrete types and save the list of results
Expand Down Expand Up @@ -753,7 +755,7 @@ impl TypingContext {
.cloned()
.zip([&node_io.call_argument].into_iter().chain(&node_io.inputs).cloned())
.enumerate()
.filter(|(_, (p1, p2))| !valid_subtype(p1, p2))
.filter(|(_, (p1, p2))| !valid_type(p1, p2))
.map(|(index, ty)| {
let i = node.original_location.inputs(index).min_by_key(|s| s.node.len()).map(|s| s.index).unwrap_or(index);
let i = if using_manual_composition { i } else { i + 1 };
Expand Down
4 changes: 2 additions & 2 deletions node-graph/interpreted-executor/src/node_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ fn node_registry() -> HashMap<ProtoNodeIdentifier, HashMap<NodeIOTypes, NodeCons
ProtoNodeIdentifier::new(stringify!(wgpu_executor::CreateGpuSurfaceNode<_>)),
|args| {
Box::pin(async move {
let editor_api: DowncastBothNode<(), &WasmEditorApi> = DowncastBothNode::new(args[0].clone());
let editor_api: DowncastBothNode<Context, &WasmEditorApi> = DowncastBothNode::new(args[0].clone());
let node = <wgpu_executor::CreateGpuSurfaceNode<_>>::new(editor_api);
let any: DynAnyNode<(), _, _> = graphene_std::any::DynAnyNode::new(node);
let any: DynAnyNode<Context, _, _> = graphene_std::any::DynAnyNode::new(node);
Box::new(any) as TypeErasedBox
})
},
Expand Down
Loading