Remove PortDef::into (#1671)

* remove `PortDef::into`

* changelog

CHANGELOG.md

@@ -2,6 +2,7 @@
 - Don't require `@clk` and `@reset` ports in `comb` components
 - `inline` pass supports inlining `ref` cells
 - `comb-prop`: disable rewrite from `wire.in = port` when the output of a wire is read.
+- BREAKING: Remove `PortDef::into()` because it makes it easy to miss copying attributes.
 
 
 ## 0.4.0

calyx-frontend/src/common.rs

@@ -95,7 +95,7 @@ impl GetName for Primitive {
 #[derive(Clone, Debug)]
 pub struct PortDef<W> {
     /// The name of the port.
-    pub name: Id,
+    name: Id,
     /// The width of the port. .
     pub width: W,
     /// The direction of the port. Only allowed to be [Direction::Input]
@@ -105,31 +105,29 @@ pub struct PortDef<W> {
     pub attributes: Attributes,
 }
 
-impl<I> From<(I, u64, Direction)> for PortDef<Width>
-where
-    I: Into<Id>,
-{
-    fn from(port: (I, u64, Direction)) -> Self {
-        PortDef {
-            name: port.0.into(),
-            width: Width::Const { value: port.1 },
-            direction: port.2,
-            attributes: Attributes::default(),
+impl<W> PortDef<W> {
+    pub fn new(
+        name: impl Into<Id>,
+        width: W,
+        direction: Direction,
+        attributes: Attributes,
+    ) -> Self {
+        assert!(
+            matches!(direction, Direction::Input | Direction::Output),
+            "Direction must be either Input or Output"
+        );
+
+        Self {
+            name: name.into(),
+            width,
+            direction,
+            attributes,
         }
     }
-}
 
-impl<I> From<(I, u64, Direction)> for PortDef<u64>
-where
-    I: Into<Id>,
-{
-    fn from(port: (I, u64, Direction)) -> Self {
-        PortDef {
-            name: port.0.into(),
-            width: port.1,
-            direction: port.2,
-            attributes: Attributes::default(),
-        }
+    /// Return the name of the port definition
+    pub fn name(&self) -> Id {
+        self.name
     }
 }
 
@@ -151,6 +149,18 @@ impl std::fmt::Display for Width {
     }
 }
 
+impl From<u64> for Width {
+    fn from(value: u64) -> Self {
+        Width::Const { value }
+    }
+}
+
+impl From<Id> for Width {
+    fn from(value: Id) -> Self {
+        Width::Param { value }
+    }
+}
+
 impl PortDef<Width> {
     /// Given a map from names of parameters to their values, attempt to
     /// resolve this definition.

calyx-frontend/src/parser.rs

@@ -454,15 +454,15 @@ impl CalyxParser {
         Ok(match_nodes!(
             input.into_children();
             [io_port((name, width, attributes))] => {
-                let pd = PortDef {
-                    name, width, direction: Direction::Input, attributes
-                };
+                let pd = PortDef::new(
+                    name, width, Direction::Input, attributes
+                );
                 vec![pd]
             },
             [io_port((name, width, attributes)), comma(_), inputs(rest)] => {
-                let pd = PortDef {
-                    name, width, direction: Direction::Input, attributes
-                };
+                let pd = PortDef::new(
+                    name, width, Direction::Input, attributes
+                );
                 let mut v = vec![pd];
                 v.extend(rest);
                 v
@@ -474,15 +474,15 @@ impl CalyxParser {
         Ok(match_nodes!(
             input.into_children();
             [io_port((name, width, attributes))] => {
-                let pd = PortDef {
-                    name, width, direction: Direction::Output, attributes
-                };
+                let pd = PortDef::new(
+                    name, width, Direction::Output, attributes
+                );
                 vec![pd]
             },
             [io_port((name, width, attributes)), comma(_), outputs(rest)] => {
-                let pd = PortDef {
-                    name, width, direction: Direction::Output, attributes
-                };
+                let pd = PortDef::new(
+                    name, width, Direction::Output, attributes
+                );
                 let mut v = vec![pd];
                 v.extend(rest);
                 v
@@ -1094,14 +1094,14 @@ impl CalyxParser {
                     Err(input.error("Static Component must have defined control"))?;
                 }
                 let (continuous_assignments, groups, static_groups) = connections;
-                let sig = sig.into_iter().map(|PortDef { name, width, direction, attributes }| {
-                    if let Width::Const { value } = width {
-                        Ok(PortDef {
-                            name,
-                            width: value,
-                            direction,
-                            attributes
-                        })
+                let sig = sig.into_iter().map(|pd| {
+                    if let Width::Const { value } = pd.width {
+                        Ok(PortDef::new(
+                            pd.name(),
+                            value,
+                            pd.direction,
+                            pd.attributes
+                        ))
                     } else {
                         Err(input.error("Components cannot use parameters"))
                     }
@@ -1127,14 +1127,14 @@ impl CalyxParser {
                 control(control)
             ] => {
                 let (continuous_assignments, groups, static_groups) = connections;
-                let sig = sig.into_iter().map(|PortDef { name, width, direction, attributes }| {
-                    if let Width::Const { value } = width {
-                        Ok(PortDef {
-                            name,
-                            width: value,
-                            direction,
-                            attributes
-                        })
+                let sig = sig.into_iter().map(|pd| {
+                    if let Width::Const { value } = pd.width {
+                        Ok(PortDef::new(
+                            pd.name(),
+                            value,
+                            pd.direction,
+                            pd.attributes
+                        ))
                     } else {
                         Err(input.error("Components cannot use parameters"))
                     }
@@ -1161,14 +1161,14 @@ impl CalyxParser {
                 control(control),
             ] => {
                 let (continuous_assignments, groups, static_groups) = connections;
-                let sig = sig.into_iter().map(|PortDef { name, width, direction, attributes }| {
-                    if let Width::Const { value } = width {
-                        Ok(PortDef {
-                            name,
-                            width: value,
-                            direction,
-                            attributes
-                        })
+                let sig = sig.into_iter().map(|pd| {
+                    if let Width::Const { value } = pd.width {
+                        Ok(PortDef::new(
+                            pd.name(),
+                            value,
+                            pd.direction,
+                            pd.attributes
+                        ))
                     } else {
                         Err(input.error("Components cannot use parameters"))
                     }

calyx-ir/src/builder.rs

@@ -179,12 +179,12 @@ impl<'a> Builder<'a> {
         let cell = Self::cell_from_signature(
             name,
             ir::CellType::Constant { val, width },
-            vec![ir::PortDef {
-                name: "out".into(),
+            vec![ir::PortDef::new(
+                ir::Id::from("out"),
                 width,
-                direction: ir::Direction::Output,
-                attributes: ir::Attributes::default(),
-            }],
+                ir::Direction::Output,
+                ir::Attributes::default(),
+            )],
         );
 
         // Add constant to the Component.
@@ -347,23 +347,16 @@ impl<'a> Builder<'a> {
         ports: Vec<ir::PortDef<u64>>,
     ) -> RRC<ir::Cell> {
         let cell = Rc::new(RefCell::new(ir::Cell::new(name, typ)));
-        ports.into_iter().for_each(
-            |PortDef {
-                 name,
-                 width,
-                 direction,
-                 attributes,
-             }| {
-                let port = Rc::new(RefCell::new(ir::Port {
-                    name,
-                    width,
-                    direction,
-                    parent: ir::PortParent::Cell(WRC::from(&cell)),
-                    attributes,
-                }));
-                cell.borrow_mut().ports.push(port);
-            },
-        );
+        ports.into_iter().for_each(|pd| {
+            let port = Rc::new(RefCell::new(ir::Port {
+                name: pd.name(),
+                width: pd.width,
+                direction: pd.direction,
+                parent: ir::PortParent::Cell(WRC::from(&cell)),
+                attributes: pd.attributes,
+            }));
+            cell.borrow_mut().ports.push(port);
+        });
         cell
     }
 }

calyx-ir/src/component.rs

@@ -68,7 +68,7 @@ pub struct Component {
 impl Component {
     /// Extend the signature with interface ports if they are missing.
     pub(super) fn extend_signature(sig: &mut Vec<PortDef<u64>>) {
-        let port_names: HashSet<_> = sig.iter().map(|pd| pd.name).collect();
+        let port_names: HashSet<_> = sig.iter().map(|pd| pd.name()).collect();
         let mut namegen = NameGenerator::with_prev_defined_names(port_names);
         for (attr, width, direction) in INTERFACE_PORTS.iter() {
             // Check if there is already another interface port defined for the
@@ -77,12 +77,12 @@ impl Component {
                 let mut attributes = Attributes::default();
                 attributes.insert(*attr, 1);
                 let name = Id::from(attr.to_string());
-                sig.push(PortDef {
-                    name: namegen.gen_name(name.to_string()),
-                    width: *width,
-                    direction: direction.clone(),
+                sig.push(PortDef::new(
+                    namegen.gen_name(name.to_string()),
+                    *width,
+                    direction.clone(),
                     attributes,
-                });
+                ));
             }
         }
     }
@@ -108,17 +108,21 @@ impl Component {
             Self::extend_signature(&mut ports);
         }
 
-        let prev_names: HashSet<_> = ports.iter().map(|pd| pd.name).collect();
+        let prev_names: HashSet<_> = ports.iter().map(|pd| pd.name()).collect();
 
         let this_sig = Builder::cell_from_signature(
             THIS_ID.into(),
             CellType::ThisComponent,
             ports
                 .into_iter()
                 // Reverse the port directions inside the component.
-                .map(|pd| PortDef {
-                    direction: pd.direction.reverse(),
-                    ..pd
+                .map(|pd| {
+                    PortDef::new(
+                        pd.name(),
+                        pd.width,
+                        pd.direction.reverse(),
+                        pd.attributes,
+                    )
                 })
                 .collect(),
         );

calyx-ir/src/from_ast.rs

@@ -68,10 +68,10 @@ fn check_valid_port(
         if let Some(prim) = sig_ctx.lib.find_primitive(comp_name) {
             prim.signature
                 .iter()
-                .map(|port_def| port_def.name)
+                .map(|port_def| port_def.name())
                 .collect()
         } else if let Some((comp_sigs, _)) = sig_ctx.comp_sigs.get(&comp_name) {
-            comp_sigs.iter().map(|port_def| port_def.name).collect()
+            comp_sigs.iter().map(|port_def| port_def.name()).collect()
         } else {
             return Err(Error::undefined(
                 comp_name,
@@ -91,31 +91,23 @@ fn check_valid_port(
 
 /// Validates a component signature to make sure there are not duplicate ports.
 fn check_signature(pds: &[PortDef<u64>]) -> CalyxResult<()> {
-    let mut ports: HashSet<&Id> = HashSet::new();
-    for PortDef {
-        name, direction, ..
-    } in pds
-    {
+    let mut ports: HashSet<Id> = HashSet::new();
+    for pd in pds {
+        let name = pd.name();
         // check for uniqueness
-        match &direction {
+        match &pd.direction {
             Direction::Input => {
                 if !ports.contains(&name) {
                     ports.insert(name);
                 } else {
-                    return Err(Error::already_bound(
-                        *name,
-                        "port".to_string(),
-                    ));
+                    return Err(Error::already_bound(name, "port".to_string()));
                 }
             }
             Direction::Output => {
                 if !ports.contains(&name) {
                     ports.insert(name);
                 } else {
-                    return Err(Error::already_bound(
-                        *name,
-                        "port".to_string(),
-                    ));
+                    return Err(Error::already_bound(name, "port".to_string()));
                 }
             }
             Direction::Inout => {