Rust: merge structurally equal types in bindgen #1468
Conversation
| // When we allow importing/exporting the same interface multiple times, we need to update this code | ||
| if !self.types.get(ty_id).has_resource | ||
| && iface_key.is_some() | ||
| && let Some(true) = self.interface_last_seen_as_import.get(&iface_id) |
There was a problem hiding this comment.
It might be my memory failing me but I'm not entirely sure what these clauses are doing. Could you expand the comment to explain more what case this is handling?
There was a problem hiding this comment.
This branch means that we are processing an interface that has been imported before. With the current WIT parser, it means we are importing and exporting the same interface. For export interface, if we can't find an equal type, we can still alias the type to the imported interface (for non-resource types).
There was a problem hiding this comment.
Makes sense yeah, mind expanding the comment here with some of that info?
| } | ||
| } | ||
|
|
||
| pub fn type_alias_to_eqaul_type( |
| self.equal_types.find(*a) == self.equal_types.find(*b) | ||
| } | ||
| (Type::ErrorContext, Type::ErrorContext) => todo!(), | ||
| _ => a == b, |
There was a problem hiding this comment.
Personally I prefer to keep things exhaustive where possible to avoid _ => { /* ... */ } catch-call cases (same with _ => false above)
Could this be changed to soemthing like:
(Type::Id(a), Type::Id(b)) => { /* logic */ }
(Type::Id(_), _) => false,
(a @ Type::{a, b, c, d}, b) => a == b,That way adding new types is forced to update this to ensure it handles new types appropriately.
| match (a, b) { | ||
| (Some(a), Some(b)) => self.types_equal(resolve, a, b), | ||
| (None, None) => true, | ||
| _ => false, |
There was a problem hiding this comment.
Similar to above, can the _ case be filled out to make this match exhaustive?
| _ => false, | ||
| } | ||
| } | ||
| fn is_resource_like_type(&self, ty: &TypeDefKind) -> bool { |
There was a problem hiding this comment.
I think this might work better if this function didn't exist and it were folded directly into types_equal or is_structurally_equal. In a sense resources shouldn't be handled any differently than anything else, it's just that if they have different origin ids then they're different. I think that should fit into the rest of the logic here pretty well.
There was a problem hiding this comment.
if they have different origin ids then they're different.
Not quite. It also depends on how the world import/export these interfaces.
interface A {
resource R;
}
interface B {
use A.{R};
}
If we import A; import B;, then A.R is equal to B.R. If we import A; export B;, then A.R is not equal to B.R.
In the future, if we allow import B; import B as B1; (which is possible in WAT already), then B.R is not equal to B1.R.
It would be great if we can specify the resource equality precisely in types_equal, but since it depends on the world and its transitive dependencies, it feels complicated. This function is trying to be conservative, and say that any types that contain resources are not equal. So that we don't need to handle all these tricky cases.
There was a problem hiding this comment.
For import A; export B;, in that case R is actually the same in both cases. The export transitively refers to the import (extra exports aren't injected).
For supporting import B as B1 that'll require pretty invasive changes, in my opinion, to wit-parser to actually support it. Ideally if you did import A; export A today that would result in two separate InterfaceIds (and TypeIds internally) for the two interfaces but it currently doesn't do that.
I agree the way things are handled today is weird since import A; export A; uses the same TypeIds which isn't great. If that's the rationale for keeping it the way it is, mind leaving a comment which explains that this should in theory be fancier but it's hard today?
| fn is_resource_like_type(&self, ty: &TypeDefKind) -> bool { | ||
| match ty { | ||
| TypeDefKind::Resource | TypeDefKind::Handle(_) => true, | ||
| TypeDefKind::Future(_) | TypeDefKind::Stream(_) => true, |
There was a problem hiding this comment.
This part can be safely omitted since futures/streams are safe to deduplicate. It's just resources that can't be deduplicated.
There was a problem hiding this comment.
I'm not very familiar with futures/streams, with this WIT
interface A {
record R1 { a: stream };
record R2 { a: stream };
}
interface B {
record R1 { a: stream };
record R2 { a: stream };
}
world root {
import A;
export B;
}
Can A.R1 and A.R2 be merged? What about A.R1 and B.R1? Also self.types.has_resource includes both resource, future and stream types, which suggests they are all nominal types?
There was a problem hiding this comment.
Yes, all of those types are safe to merge. The has_resource bit might be for destructors and/or "needs cleanup", but I sort of forget... From the perspective of the component model and generated bindings those all of the above generate the same type.
| /// Find all structurally equal types and only generate one type definition for | ||
| /// each equivalence class. Other types in the same class will be type aliases to the | ||
| /// generated type. This avoids clone when converting between types that are | ||
| /// structurally equal, which is useful when import and export the same interface. | ||
| /// | ||
| /// Types containing resource, future, or stream are never considered equal. | ||
| #[cfg_attr(feature = "clap", arg(long))] | ||
| pub merge_structurally_equal_types: bool, |
There was a problem hiding this comment.
This I think would be reasonable to turn on by default, but are you hesitant to enable it by default?
There was a problem hiding this comment.
Yeah, sure. Happy to make it on by default. Just being cautious here, not to change the production behavior.
There was a problem hiding this comment.
Ah yeah that's find with semver and such, and this is something that I've heard others asking for and I don't know why it wouldn't be the default, so I think it's reasonable to have it on-by-default.
2 participants
Fix #1415
Add
merge_structurally_equal_typesflag to merge structurally equal types.collect_equal_typestakes O(n^2) time. If performance is a concern, I can define a hash function to bring down the time to O(n).