Merge pull request #250 from posit-dev/bugfix/variable-dims

Fix lengths of tabular objects in variables pane

Author: lionel-

crates/ark/src/data_explorer/r_data_explorer.rs

@@ -24,6 +24,7 @@ use amalthea::comm::data_explorer_comm::TableState;
 use amalthea::comm::event::CommManagerEvent;
 use amalthea::socket::comm::CommInitiator;
 use amalthea::socket::comm::CommSocket;
+use anyhow::anyhow;
 use anyhow::bail;
 use crossbeam::channel::Sender;
 use harp::exec::RFunction;
@@ -34,6 +35,7 @@ use harp::utils::r_is_object;
 use harp::utils::r_is_s4;
 use harp::utils::r_typeof;
 use harp::vector::formatted_vector::FormattedVector;
+use harp::TableInfo;
 use libr::*;
 use serde::Deserialize;
 use serde::Serialize;
@@ -195,6 +197,8 @@ impl RDataExplorer {
             let table = self.table.get().clone();
             let object = *table;
 
+            let info = table_info_or_bail(object)?;
+
             let harp::TableInfo {
                 kind,
                 dims:
@@ -203,7 +207,7 @@ impl RDataExplorer {
                         num_cols: total_num_columns,
                     },
                 col_names: column_names,
-            } = harp::table_info(object)?;
+            } = info;
 
             let lower_bound = cmp::min(start_index, total_num_columns) as isize;
             let upper_bound = cmp::min(total_num_columns, start_index + num_columns) as isize;
@@ -258,7 +262,7 @@ impl RDataExplorer {
                     num_cols: num_columns,
                 },
             col_names: _,
-        } = harp::table_info(object)?;
+        } = table_info_or_bail(object)?;
 
         let state = TableState {
             table_shape: TableShape {
@@ -280,14 +284,16 @@ impl RDataExplorer {
         let table = self.table.get().clone();
         let object = *table;
 
+        let info = table_info_or_bail(object)?;
+
         let harp::TableInfo {
             dims:
                 harp::TableDim {
                     num_rows: total_num_rows,
                     num_cols: total_num_cols,
                 },
             ..
-        } = harp::table_info(object)?;
+        } = info;
 
         let lower_bound = cmp::min(row_start_index, total_num_rows) as isize;
         let upper_bound = cmp::min(row_start_index + num_rows, total_num_rows) as isize;
@@ -405,6 +411,10 @@ fn display_type(x: SEXP) -> ColumnSchemaTypeDisplay {
     }
 }
 
+fn table_info_or_bail(x: SEXP) -> anyhow::Result<TableInfo> {
+    harp::table_info(x).ok_or(anyhow!("Unsupported type for data viewer"))
+}
+
 #[harp::register]
 pub unsafe extern "C" fn ps_view_data_frame(x: SEXP, title: SEXP) -> anyhow::Result<SEXP> {
     let x = RObject::new(x);

crates/ark/src/variables/variable.rs

@@ -547,29 +547,25 @@ impl PositronVariable {
     }
 
     fn variable_length(x: SEXP) -> usize {
+        // Check for tabular data
+        if let Some(info) = harp::table_info(x) {
+            return info.dims.num_cols as usize;
+        }
+
+        // Otherwise treat as vector
         let rtype = r_typeof(x);
         match rtype {
-            LGLSXP | RAWSXP | INTSXP | REALSXP | CPLXSXP | STRSXP => unsafe {
+            LGLSXP | RAWSXP | INTSXP | REALSXP | CPLXSXP | STRSXP | LISTSXP => unsafe {
                 Rf_xlength(x) as usize
             },
             VECSXP => unsafe {
-                if r_inherits(x, "POSIXlt") {
+                // TODO: Support vctrs types like record vectors
+                if r_inherits(x, "POSIXlt") && r_typeof(x) == VECSXP && r_length(x) > 0 {
                     Rf_xlength(VECTOR_ELT(x, 0)) as usize
-                } else if r_is_data_frame(x) {
-                    let dim = RFunction::new("base", "dim.data.frame")
-                        .add(x)
-                        .call()
-                        .unwrap();
-
-                    INTEGER_ELT(*dim, 0) as usize
                 } else {
                     Rf_xlength(x) as usize
                 }
             },
-            LISTSXP => match pairlist_size(x) {
-                Ok(n) => n as usize,
-                Err(_) => 0,
-            },
             _ => 0,
         }
     }

crates/harp/src/table.rs

@@ -2,6 +2,7 @@ use libr::*;
 
 use crate::exec::RFunction;
 use crate::exec::RFunctionExt;
+use crate::object::r_length;
 use crate::utils::r_is_data_frame;
 use crate::utils::r_is_matrix;
 use crate::utils::r_typeof;
@@ -19,17 +20,19 @@ pub struct TableInfo {
     pub col_names: ColumnNames,
 }
 
-pub fn table_info(x: SEXP) -> anyhow::Result<TableInfo> {
+// TODO: Might want to encode as types with methods so that we can make
+// assumptions about memory layout more safely. Also makes it possible
+// to compute properties more lazily.
+pub fn table_info(x: SEXP) -> Option<TableInfo> {
     if r_is_data_frame(x) {
-        return df_info(x);
+        return df_info(x).ok();
     }
 
     if r_is_matrix(x) {
-        return mat_info(x);
+        return mat_info(x).ok();
     }
 
-    // TODO: better error message
-    anyhow::bail!("Unsupported type for data viewer");
+    None
 }
 
 pub fn df_info(x: SEXP) -> anyhow::Result<TableInfo> {
@@ -81,6 +84,14 @@ pub fn mat_dim(x: SEXP) -> TableDim {
     unsafe {
         let dims = Rf_getAttrib(x, R_DimSymbol);
 
+        // Might want to return an error instead, or take a strongly typed input
+        if r_typeof(dims) != INTSXP || r_length(dims) != 2 {
+            return TableDim {
+                num_rows: r_length(x) as i32,
+                num_cols: 1,
+            };
+        }
+
         TableDim {
             num_rows: INTEGER_ELT(dims, 0),
             num_cols: INTEGER_ELT(dims, 1),