Overhaul the expansion system #728

wycats · 2019-09-28T22:57:19Z

The main thrust of this (very large) commit is an overhaul of the
expansion system.

The parsing pipeline is:

Lightly parse the source file for atoms, basic delimiters and pipeline
structure into a token tree
Expand the token tree into a HIR (high-level intermediate
representation) based upon the baseline syntax rules for expressions
and the syntactic shape of commands.

Somewhat non-traditionally, nu doesn't have an AST at all. It goes
directly from the token tree, which doesn't represent many important
distinctions (like the difference between hello and 5KB) directly
into a high-level representation that doesn't have a direct
correspondence to the source code.

At a high level, nu commands work like macros, in the sense that the
syntactic shape of the invocation of a command depends on the
definition of a command.

However, commands do not have the ability to perform unrestricted
expansions of the token tree. Instead, they describe their arguments in
terms of syntactic shapes, and the expander expands the token tree into
HIR based upon that definition.

For example, the where command says that it takes a block as its first
required argument, and the description of the block syntactic shape
expands the syntax cpu > 10 into HIR that represents
{ $it.cpu > 10 }.

This commit overhauls that system so that the syntactic shapes are
described in terms of a few new traits (ExpandSyntax and
ExpandExpression are the primary ones) that are more composable than
the previous system.

The first big win of this new system is the addition of the ColumnPath
shape, which looks like cpu."max ghz" or package.version.
Previously, while a variable path could look like $it.cpu."max ghz",
the tail of a variable path could not be easily reused in other
contexts. Now, that tail is its own syntactic shape, and it can be used
as part of a command's signature.

This cleans up commands like inc, add and edit as well as
shorthand blocks, which can now look like | where cpu."max ghz" > 10

sophiajt

Mostly nits, though I found a few places where functionality got removed/commented out.

Might be good to add some comments either here or in the book about what these new concepts mean (eg "what's a shape?", "what's a tail?")

sophiajt · 2019-09-28T23:10:03Z

src/cli.rs

@@ -75,7 +80,7 @@ fn load_plugin(path: &std::path::Path, context: &mut Context) -> Result<(), Shel
                        let name = params.name.clone();
                        let fname = fname.to_string();

-                        if context.has_command(&name) {
+                        if let Some(_) = context.get_command(&name) {


has_command seems okay here

sophiajt · 2019-09-28T23:10:35Z

src/cli.rs

-                    let writer = host.err_termcolor();
-                    line.push_str(" ");
-                    let files = crate::parser::Files::new(line);
-                    let _ = std::panic::catch_unwind(move || {


this was catching panics in the error reporter. Is the the error reporting crate in a state we can take this out now?

sophiajt · 2019-09-28T23:13:34Z

src/cli.rs

@@ -514,16 +519,24 @@ async fn process_line(readline: Result<String, ReadlineError>, ctx: &mut Context
                input = match (item, next) {
                    (None, _) => break,

+                    (Some(ClassifiedCommand::Dynamic(_)), _)
+                    | (_, Some(ClassifiedCommand::Dynamic(_))) => {


Is this from an idea that didn't get fleshed out?

sophiajt · 2019-09-28T23:15:39Z

src/commands/autoview.rs

-                        }
-                    }
-                }
+            // } else if is_single_origined_text_value(&input) {


This looks wrong. It's going to break textview

sophiajt · 2019-09-28T23:17:19Z

src/commands/fetch.rs

@@ -53,7 +53,7 @@ fn run(
    };
    let path_buf = path.as_path()?;
    let path_str = path_buf.display().to_string();
-    let path_span = path.span();
+    let path_span = path.tag.span;


curious - we could keep the .span() method and keep this cleaner.

sophiajt · 2019-09-28T23:43:14Z

src/parser/parse/parser.rs

+    let next_char = &input.fragment.chars().nth(0);
+    let prev_char = last.fragment.chars().nth(0);
+
+    // if let (Some(prev), Some(next)) = (prev_char, next_char) {


should maybe remove this if we're not using it

sophiajt · 2019-09-28T23:44:59Z

src/parser/parse/parser.rs

-    fn test_size() {
-        assert_leaf! {
-            parsers [ size ]
-            "123MB" -> 0..5 { Size(RawNumber::int((0, 3, test_uuid())).item, Unit::MB) }


I'm unclear on what's happening with sizes.

sophiajt · 2019-09-28T23:46:29Z

src/parser/parse/parser.rs

        let _ = pretty_env_logger::try_init();

-        assert_eq!(


Can you say a bit about removing/changing these tests?

sophiajt · 2019-09-28T23:46:39Z

src/parser/parse/parser.rs

-    fn apply<T>(
-        f: impl Fn(NomSpan) -> Result<(NomSpan, T), nom::Err<(NomSpan, nom::error::ErrorKind)>>,
+    // #[test]
+    // fn test_pseudo_paths() {


sophiajt · 2019-09-28T23:47:15Z

src/parser/parse/pipeline.rs

-    pub(crate) parts: Vec<PipelineElement>,
-    pub(crate) post_ws: Option<Tag>,
+    pub(crate) parts: Vec<Tagged<PipelineElement>>,
+    // pub(crate) post_ws: Option<Tag>,


could probably remove if we're not using

gitpod-io · 2019-10-08T05:37:00Z

The main thrust of this (very large) commit is an overhaul of the expansion system. The parsing pipeline is: - Lightly parse the source file for atoms, basic delimiters and pipeline structure into a token tree - Expand the token tree into a HIR (high-level intermediate representation) based upon the baseline syntax rules for expressions and the syntactic shape of commands. Somewhat non-traditionally, nu doesn't have an AST at all. It goes directly from the token tree, which doesn't represent many important distinctions (like the difference between `hello` and `5KB`) directly into a high-level representation that doesn't have a direct correspondence to the source code. At a high level, nu commands work like macros, in the sense that the syntactic shape of the invocation of a command depends on the definition of a command. However, commands do not have the ability to perform unrestricted expansions of the token tree. Instead, they describe their arguments in terms of syntactic shapes, and the expander expands the token tree into HIR based upon that definition. For example, the `where` command says that it takes a block as its first required argument, and the description of the block syntactic shape expands the syntax `cpu > 10` into HIR that represents `{ $it.cpu > 10 }`. This commit overhauls that system so that the syntactic shapes are described in terms of a few new traits (`ExpandSyntax` and `ExpandExpression` are the primary ones) that are more composable than the previous system. The first big win of this new system is the addition of the `ColumnPath` shape, which looks like `cpu."max ghz"` or `package.version`. Previously, while a variable path could look like `$it.cpu."max ghz"`, the tail of a variable path could not be easily reused in other contexts. Now, that tail is its own syntactic shape, and it can be used as part of a command's signature. This cleans up commands like `inc`, `add` and `edit` as well as shorthand blocks, which can now look like `| where cpu."max ghz" > 10`

gitpod-io · 2019-10-10T21:07:21Z

This commit replaces the previous naive coloring system with a coloring system that is more aligned with the parser. The main benefit of this change is that it allows us to use parsing rules to decide how to color tokens. For example, consider the following syntax: ``` $ ps | where cpu > 10 ``` Ideally, we could color `cpu` like a column name and not a string, because `cpu > 10` is a shorthand block syntax that expands to `{ $it.cpu > 10 }`. The way that we know that it's a shorthand block is that the `where` command declares that its first parameter is a `SyntaxShape::Block`, which allows the shorthand block form. In order to accomplish this, we need to color the tokens in a way that corresponds to their expanded semantics, which means that high-fidelity coloring requires expansion. This commit adds a `ColorSyntax` trait that corresponds to the `ExpandExpression` trait. The semantics are fairly similar, with a few differences. First `ExpandExpression` consumes N tokens and returns a single `hir::Expression`. `ColorSyntax` consumes N tokens and writes M `FlatShape` tokens to the output. Concretely, for syntax like `[1 2 3]` - `ExpandExpression` takes a single token node and produces a single `hir::Expression` - `ColorSyntax` takes the same token node and emits 7 `FlatShape`s (open delimiter, int, whitespace, int, whitespace, int, close delimiter) Second, `ColorSyntax` is more willing to plow through failures than `ExpandExpression`. In particular, consider syntax like ``` $ ps | where cpu > ``` In this case - `ExpandExpression` will see that the `where` command is expecting a block, see that it's not a literal block and try to parse it as a shorthand block. It will successfully find a member followed by an infix operator, but not a following expression. That means that the entire pipeline part fails to parse and is a syntax error. - `ColorSyntax` will also try to parse it as a shorthand block and ultimately fail, but it will fall back to "backoff coloring mode", which parsing any unidentified tokens in an unfallible, simple way. In this case, `cpu` will color as a string and `>` will color as an operator. Finally, it's very important that coloring a pipeline infallibly colors the entire string, doesn't fail, and doesn't get stuck in an infinite loop. In order to accomplish this, this PR separates `ColorSyntax`, which is infallible from `FallibleColorSyntax`, which might fail. This allows the type system to let us know if our coloring rules bottom out at at an infallible rule. It's not perfect: it's still possible for the coloring process to get stuck or consume tokens non-atomically. I intend to reduce the opportunity for those problems in a future commit. In the meantime, the current system catches a number of mistakes (like trying to use a fallible coloring rule in a loop without thinking about the possibility that it will never terminate).

sophiajt reviewed Sep 28, 2019

View reviewed changes

sophiajt changed the title ~~Overhaul the expansion system~~ [DON'T MERGE] Overhaul the expansion system Oct 10, 2019

wycats force-pushed the better-pseudo-blocks branch from c4cc508 to 463df69 Compare October 10, 2019 18:28

wycats force-pushed the better-pseudo-blocks branch from ee14bbc to c2c10e2 Compare October 11, 2019 02:55

sophiajt merged commit 3317b13 into master Oct 11, 2019

sophiajt changed the title ~~[DON'T MERGE] Overhaul the expansion system~~ Overhaul the expansion system Oct 11, 2019

Falcz mentioned this pull request Oct 15, 2019

Convert span-less errors to spanned errors #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul the expansion system #728

Overhaul the expansion system #728

wycats commented Sep 28, 2019

sophiajt left a comment

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

sophiajt Sep 28, 2019

gitpod-io bot commented Oct 8, 2019 •

edited

gitpod-io bot commented Oct 10, 2019 •

edited

Overhaul the expansion system #728

Overhaul the expansion system #728

Conversation

wycats commented Sep 28, 2019

sophiajt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitpod-io bot commented Oct 8, 2019 • edited

gitpod-io bot commented Oct 10, 2019 • edited

gitpod-io bot commented Oct 8, 2019 •

edited

gitpod-io bot commented Oct 10, 2019 •

edited