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

Correctly display connections to lambdas #7550

Merged
merged 7 commits into from
Aug 11, 2023
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@
the selected entry in the component browser. Clear separating lines between
method arguments were added. The node selection was made easier with
additional thick interactive selection border.
- [Connections to lamdas are displayed correctly][7550]. It is possible to drag
a connection to any expression inside the lambda body.

[5910]: https://github.com/enso-org/enso/pull/5910
[6279]: https://github.com/enso-org/enso/pull/6279
Expand All @@ -239,6 +241,7 @@
[7372]: https://github.com/enso-org/enso/pull/7372
[7337]: https://github.com/enso-org/enso/pull/7337
[7311]: https://github.com/enso-org/enso/pull/7311
[7550]: https://github.com/enso-org/enso/pull/7550

#### EnsoGL (rendering engine)

Expand Down
23 changes: 23 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions app/gui/language/span-tree/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ parser = { path = "../parser" }

[dev-dependencies]
wasm-bindgen-test = { workspace = true }
pretty_assertions = "1.4"
6 changes: 6 additions & 0 deletions app/gui/language/span-tree/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,10 @@ impl<Parent> ChildBuilder<Parent> {
self.built.node.ast_id = Some(id);
self
}

/// Set tree type for this node.
pub fn set_tree_type(mut self, r#type: Option<ast::TreeType>) -> Self {
self.built.node.tree_type = r#type;
self
}
}
106 changes: 98 additions & 8 deletions app/gui/language/span-tree/src/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,7 @@ fn generate_node_for_ast(
match ast.shape() {
ast::Shape::Prefix(_) =>
ast::prefix::Chain::from_ast(ast).unwrap().generate_node(kind, context),
ast::Shape::Tree(tree) if tree.type_info != ast::TreeType::Lambda =>
tree_generate_node(tree, kind, context, ast.id),
ast::Shape::Tree(tree) => tree_generate_node(tree, kind, context, ast.id),
ast::Shape::Block(block) => block_generate_node(block, kind, context, ast.id),
_ => {
let size = (ast.repr_len().value as i32).byte_diff();
Expand Down Expand Up @@ -818,11 +817,44 @@ fn generate_trailing_expected_arguments(
})
}

/// A single child node produced out of the lambda argument and the `->` token.
#[derive(Debug)]
struct FoldedLambdaArguments {
/// Both the lambda argument and the `->` token, as a single [`node::Kind::Token`] node.
child: node::Child,
Comment on lines +823 to +824
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little afraid of the consequences of folding all arguments and the arrow together into a single token. That means we won't be able to create any useful widgets there, like the ability to add more arguments or apply custom styling. But I guess we can expand this logic once we need it, and likely the span-tree will be removed at this point anyway. So I think it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that we were not planning to support connections to lambdas at all, so yes, let's think about it when the time comes :)

/// The number of tree nodes that were folded into the `child`.
nodes_replaced: usize,
}


// =========================
// === SpanTree for Tree ===
// =========================
/// Fold the lambda arguments into a single [`node::Kind::Token`] node.
/// It is needed to ignore lambda arguments as connection targets, but still generate a valid
/// SpanTree from the lambda body.
fn fold_lambda_arguments(tree: &ast::Tree<Ast>) -> FoldedLambdaArguments {
let is_arrow = |span_info| matches!(span_info, SpanSeed::Token(ast::SpanSeedToken { token }) if token == "->");
let arrow_index = tree.span_info.iter().cloned().position(is_arrow).unwrap_or(0);
let bytes_till_body = tree
.span_info
.iter()
.take(arrow_index + 1)
.map(|raw_span_info| match raw_span_info {
SpanSeed::Space(ast::SpanSeedSpace { space }) => ByteDiff::from(space),
SpanSeed::Token(ast::SpanSeedToken { token }) => ByteDiff::from(token.len()),
SpanSeed::Child(ast::SpanSeedChild { node }) => node.repr_len().to_diff(),
})
.sum::<ByteDiff>();
let size = bytes_till_body;
let kind = node::Kind::Token;
let node = Node::new().with_kind(kind).with_size(size);
let ast_crumbs = vec![TreeCrumb { index: 0 }.into()];
let nodes_replaced = arrow_index + 1;
let child = node::Child {
node,
parent_offset: ByteDiff::from(0),
sibling_offset: ByteDiff::from(0),
ast_crumbs,
};
FoldedLambdaArguments { child, nodes_replaced }
}

fn tree_generate_node(
tree: &ast::Tree<Ast>,
Expand All @@ -846,7 +878,19 @@ fn tree_generate_node(

let last_token_index =
tree.span_info.iter().rposition(|span| matches!(span, SpanSeed::Token(_)));
for (index, raw_span_info) in tree.span_info.iter().enumerate() {

// If the node is a lambda, we fold the lambda arguments into a single child node,
// and then continue handling the lambda body as usual.
let skip = if tree.type_info == ast::TreeType::Lambda {
let FoldedLambdaArguments { child, nodes_replaced } = fold_lambda_arguments(tree);
parent_offset += child.node.size;
children.push(child);
nodes_replaced
} else {
0
};
for (index, raw_span_info) in tree.span_info.iter().skip(skip).enumerate() {
let index = index + skip;
match raw_span_info {
SpanSeed::Space(ast::SpanSeedSpace { space }) => {
parent_offset += ByteDiff::from(space);
Expand Down Expand Up @@ -985,6 +1029,7 @@ mod test {
use ast::Crumbs;
use ast::IdMap;
use parser::Parser;
use pretty_assertions::assert_eq;


/// A helper function which removes information about expression id from thw tree rooted at
Expand Down Expand Up @@ -1150,6 +1195,10 @@ mod test {
#[test]
fn generating_span_tree_for_lambda() {
let parser = Parser::new();


// === Simple lambda ===

let ast = parser.parse_line_ast("foo a-> b + c").unwrap();
let mut tree: SpanTree = ast.generate_tree(&context::Empty).unwrap();
clear_expression_ids(&mut tree.root);
Expand All @@ -1158,10 +1207,51 @@ mod test {
let expected = TreeBuilder::new(13)
.add_leaf(0, 3, node::Kind::Operation, PrefixCrumb::Func)
.add_empty_child(3, BeforeArgument(0))
.add_leaf(4, 9, node::Kind::prefix_argument(), PrefixCrumb::Arg)
.add_child(4, 9, node::Kind::prefix_argument(), PrefixCrumb::Arg)
.set_tree_type(Some(ast::TreeType::Lambda))
.add_leaf(0, 3, node::Kind::Token, TreeCrumb { index: 0 })
.add_child(4, 5, node::Kind::argument(), TreeCrumb { index: 3 })
.add_empty_child(0, BeforeArgument(0))
.add_leaf(0, 1, node::Kind::argument(), InfixCrumb::LeftOperand)
.add_leaf(2, 1, node::Kind::Operation, InfixCrumb::Operator)
.add_empty_child(3, BeforeArgument(1))
.add_leaf(4, 1, node::Kind::argument(), InfixCrumb::RightOperand)
.add_empty_child(5, Append)
.done()
.done()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add also a test case for lambda like a -> b -> a + b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.add_empty_child(13, Append)
.build();
assert_eq!(expected, tree);


// === Lambda with two arguments ===

let ast = parser.parse_line_ast("foo a->b-> a + b").unwrap();
let mut tree: SpanTree = ast.generate_tree(&context::Empty).unwrap();
clear_expression_ids(&mut tree.root);
clear_parameter_infos(&mut tree.root);

let expected = TreeBuilder::new(16)
.add_leaf(0, 3, node::Kind::Operation, PrefixCrumb::Func)
.add_empty_child(3, BeforeArgument(0))
.add_child(4, 12, node::Kind::prefix_argument(), PrefixCrumb::Arg)
.set_tree_type(Some(ast::TreeType::Lambda))
.add_leaf(0, 3, node::Kind::Token, TreeCrumb { index: 0 })
.add_child(3, 9, node::Kind::argument(), TreeCrumb { index: 2 })
.set_tree_type(Some(ast::TreeType::Lambda))
.add_leaf(0, 3, node::Kind::Token, TreeCrumb { index: 0 })
.add_child(4, 5, node::Kind::argument(), TreeCrumb { index: 3 })
.add_empty_child(0, BeforeArgument(0))
.add_leaf(0, 1, node::Kind::argument(), InfixCrumb::LeftOperand)
.add_leaf(2, 1, node::Kind::Operation, InfixCrumb::Operator)
.add_empty_child(3, BeforeArgument(1))
.add_leaf(4, 1, node::Kind::argument(), InfixCrumb::RightOperand)
.add_empty_child(5, Append)
.done()
.done()
.done()
.add_empty_child(16, Append)
.build();
assert_eq!(expected, tree);
}

Expand Down
1 change: 0 additions & 1 deletion app/gui/view/src/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::notification::logged::UpdateOptions;
use ensogl::application::Application;



// ==============
// === Export ===
// ==============
Expand Down
1 change: 0 additions & 1 deletion app/gui/view/src/notification/logged.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use crate::notification::js::HandleJsError;
use uuid::Uuid;



// ==============
// === Export ===
// ==============
Expand Down
7 changes: 7 additions & 0 deletions lib/rust/text/src/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::index::*;
use crate::prelude::*;

use enso_types::unit;
use std::iter::Sum;



Expand Down Expand Up @@ -118,6 +119,12 @@ impl SubAssign<Bytes> for ByteDiff {
}
}

impl Sum<ByteDiff> for ByteDiff {
fn sum<I: Iterator<Item = ByteDiff>>(iter: I) -> Self {
iter.fold(ByteDiff(0), |acc, x| acc + x)
}
}



// ================
Expand Down