From 3de95038b0a848e07dd10377c87e8e3ad5026408 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sat, 21 Dec 2019 11:45:07 +0100
Subject: [PATCH] Make "time" a job prefix

In particular, this allows `true && time true`, or `true; and time true`,
and both `time not true` as well as `not time true` (like bash).

time is valid only as job _prefix_, so `true | time true` could call
`/bin/time` (same in bash)

See discussion in #6442
---
 src/exec.cpp                   |  2 ++
 src/highlight.cpp              |  4 +++
 src/parse_constants.h          |  5 +++-
 src/parse_execution.cpp        | 49 ++++++++++++++--------------------
 src/parse_grammar.h            | 17 ++++++++----
 src/parse_grammar_elements.inc |  1 +
 src/parse_productions.cpp      | 17 +++++++++---
 src/parse_tree.cpp             |  2 +-
 src/proc.h                     |  3 +++
 src/timer.cpp                  | 19 +++++++++++++
 src/timer.h                    |  2 ++
 src/tnode.cpp                  |  4 +--
 tests/checks/time.fish         | 18 +++++++++++++
 13 files changed, 101 insertions(+), 42 deletions(-)

diff --git a/src/exec.cpp b/src/exec.cpp
index 0b6e23593..351a67a18 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -44,6 +44,7 @@
 #include "reader.h"
 #include "redirection.h"
 #include "signal.h"
+#include "timer.h"
 #include "trace.h"
 #include "wutil.h"  // IWYU pragma: keep
 
@@ -1049,6 +1050,7 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const job_lineage_t
         parser.set_last_statuses(statuses_t::just(status));
         return false;
     }
+    cleanup_t timer = push_timer(j->flags().has_time_prefix);
 
     // Get the deferred process, if any. We will have to remember its pipes.
     autoclose_pipes_t deferred_pipes;
diff --git a/src/highlight.cpp b/src/highlight.cpp
index a9e0550bd..a4a230797 100644
--- a/src/highlight.cpp
+++ b/src/highlight.cpp
@@ -1211,6 +1211,10 @@ highlighter_t::color_array_t highlighter_t::highlight() {
                 this->color_node(node, highlight_role_t::statement_terminator);
                 break;
             }
+            case symbol_optional_time: {
+                this->color_node(node, highlight_role_t::operat);
+                break;
+            }
             case symbol_plain_statement: {
                 tnode_t<g::plain_statement> stmt(&parse_tree, &node);
                 // Get the decoration from the parent.
diff --git a/src/parse_constants.h b/src/parse_constants.h
index 441436a88..5d6ded51b 100644
--- a/src/parse_constants.h
+++ b/src/parse_constants.h
@@ -52,6 +52,7 @@ enum parse_token_type_t : uint8_t {
     symbol_redirection,
     symbol_optional_background,
     symbol_optional_newlines,
+    symbol_optional_time,
     symbol_end_command,
     // Terminal types.
     parse_token_type_string,
@@ -157,12 +158,14 @@ enum parse_job_decoration_t {
     parse_job_decoration_none,
     parse_job_decoration_and,
     parse_job_decoration_or,
-    parse_job_decoration_time,
 };
 
 // Whether a statement is backgrounded.
 enum parse_optional_background_t { parse_no_background, parse_background };
 
+// Whether a job is prefixed with "time".
+enum parse_optional_time_t { parse_optional_time_no_time, parse_optional_time_time };
+
 // Parse error code list.
 enum parse_error_code_t {
     parse_error_none,
diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index b07dc3b01..bc7401315 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -122,13 +122,14 @@ tnode_t<g::plain_statement> parse_execution_context_t::infinite_recursive_statem
     // Here's the statement node we find that's infinite recursive.
     tnode_t<grammar::plain_statement> infinite_recursive_statement;
 
-    // Get the list of plain statements.
-    // Ignore statements with decorations like 'builtin' or 'command', since those
-    // are not infinite recursion. In particular that is what enables 'wrapper functions'.
-    tnode_t<g::statement> statement = first_job.child<1>();
-    tnode_t<g::job_continuation> continuation = first_job.child<2>();
+    // Ignore the jobs variable assigment and "time" prefixes.
+    tnode_t<g::statement> statement = first_job.child<2>();
+    tnode_t<g::job_continuation> continuation = first_job.child<3>();
     const null_environment_t nullenv{};
     while (statement) {
+        // Get the list of plain statements.
+        // Ignore statements with decorations like 'builtin' or 'command', since those
+        // are not infinite recursion. In particular that is what enables 'wrapper functions'.
         tnode_t<g::plain_statement> plain_statement =
             statement.try_get_child<g::decorated_statement, 0>()
                 .try_get_child<g::plain_statement, 0>();
@@ -203,10 +204,10 @@ maybe_t<eval_result_t> parse_execution_context_t::check_end_execution() const {
 
 /// Return whether the job contains a single statement, of block type, with no redirections.
 bool parse_execution_context_t::job_is_simple_block(tnode_t<g::job> job_node) const {
-    tnode_t<g::statement> statement = job_node.child<1>();
+    tnode_t<g::statement> statement = job_node.child<2>();
 
     // Must be no pipes.
-    if (job_node.child<2>().try_get_child<g::tok_pipe, 0>()) {
+    if (job_node.child<3>().try_get_child<g::tok_pipe, 0>()) {
         return false;
     }
 
@@ -975,8 +976,10 @@ eval_result_t parse_execution_context_t::populate_not_process(
     job_t *job, process_t *proc, tnode_t<g::not_statement> not_statement) {
     auto &flags = job->mut_flags();
     flags.negate = !flags.negate;
+    auto optional_time = not_statement.require_get_child<g::optional_time, 2>();
+    if (optional_time.tag() == parse_optional_time_time) flags.has_time_prefix = true;
     return this->populate_job_process(
-        job, proc, not_statement.require_get_child<g::statement, 2>(),
+        job, proc, not_statement.require_get_child<g::statement, 3>(),
         not_statement.require_get_child<g::variable_assignments, 1>());
 }
 
@@ -1111,18 +1114,20 @@ eval_result_t parse_execution_context_t::populate_job_from_job_node(
 
     // We are going to construct process_t structures for every statement in the job. Get the first
     // statement.
-    tnode_t<g::statement> statement = job_node.child<1>();
-    tnode_t<g::variable_assignments> variable_assignments = job_node.child<0>();
+    tnode_t<g::optional_time> optional_time = job_node.child<0>();
+    tnode_t<g::variable_assignments> variable_assignments = job_node.child<1>();
+    tnode_t<g::statement> statement = job_node.child<2>();
 
     // Create processes. Each one may fail.
     process_list_t processes;
     processes.emplace_back(new process_t());
+    if (optional_time.tag() == parse_optional_time_time) j->mut_flags().has_time_prefix = true;
     eval_result_t result =
         this->populate_job_process(j, processes.back().get(), statement, variable_assignments);
 
     // Construct process_ts for job continuations (pipelines), by walking the list until we hit the
     // terminal (empty) job continuation.
-    tnode_t<g::job_continuation> job_cont = job_node.child<2>();
+    tnode_t<g::job_continuation> job_cont = job_node.child<3>();
     assert(job_cont);
     while (auto pipe = job_cont.try_get_child<g::tok_pipe, 0>()) {
         if (result != eval_result_t::ok) {
@@ -1212,7 +1217,9 @@ eval_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_node,
     // However, if there are no redirections, then we can just jump into the block directly, which
     // is significantly faster.
     if (job_is_simple_block(job_node)) {
-        tnode_t<g::variable_assignments> variable_assignments = job_node.child<0>();
+        tnode_t<g::optional_time> optional_time = job_node.child<0>();
+        cleanup_t timer = push_timer(optional_time.tag() == parse_optional_time_time);
+        tnode_t<g::variable_assignments> variable_assignments = job_node.child<1>();
         const block_t *block = nullptr;
         eval_result_t result =
             this->apply_variable_assignments(nullptr, variable_assignments, &block);
@@ -1220,7 +1227,7 @@ eval_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_node,
             if (block) parser->pop_block(block);
         });
 
-        tnode_t<g::statement> statement = job_node.child<1>();
+        tnode_t<g::statement> statement = job_node.child<2>();
         const parse_node_t &specific_statement = statement.get_child_node<0>();
         assert(specific_statement_type_is_redirectable_block(specific_statement));
         if (result == eval_result_t::ok) {
@@ -1389,7 +1396,6 @@ eval_result_t parse_execution_context_t::run_job_list(tnode_t<Type> job_list,
     static_assert(Type::token == symbol_job_list || Type::token == symbol_andor_job_list,
                   "Not a job list");
 
-    static std::vector<timer_snapshot_t> active_timers;
     eval_result_t result = eval_result_t::ok;
     while (auto job_conj = job_list.template next_in_list<g::job_conjunction>()) {
         if (auto reason = check_end_execution()) {
@@ -1399,26 +1405,11 @@ eval_result_t parse_execution_context_t::run_job_list(tnode_t<Type> job_list,
 
         // Maybe skip the job if it has a leading and/or.
         // Skipping is treated as success.
-        bool timer_started = false;
-        if (get_decorator(job_conj) == parse_job_decoration_time) {
-            active_timers.emplace_back(timer_snapshot_t::take());
-            timer_started = true;
-        }
         if (should_skip(get_decorator(job_conj))) {
             result = eval_result_t::ok;
         } else {
             result = this->run_job_conjunction(job_conj, associated_block);
         }
-        if (timer_started) {
-            auto t1 = active_timers.back();
-            active_timers.pop_back();
-            auto t2 = timer_snapshot_t::take();
-
-            // Well, this is awkward. By defining `time` as a decorator and not a built-in, there's
-            // no associated stream for its output!
-            auto output = timer_snapshot_t::print_delta(t1, t2, true);
-            std::fwprintf(stderr, L"%S\n", output.c_str());
-        }
     }
 
     // Returns the result of the last job executed or skipped.
diff --git a/src/parse_grammar.h b/src/parse_grammar.h
index 7c3062693..0373e6ece 100644
--- a/src/parse_grammar.h
+++ b/src/parse_grammar.h
@@ -210,7 +210,6 @@ DEF_ALT(job_list) {
 DEF_ALT(job_decorator) {
     using ands = single<keyword<parse_keyword_and>>;
     using ors = single<keyword<parse_keyword_or>>;
-    using times = single<keyword<parse_keyword_time>>;
     using empty = grammar::empty;
     ALT_BODY(job_decorator, ands, ors, empty);
 };
@@ -225,13 +224,20 @@ DEF_ALT(job_conjunction_continuation) {
     ALT_BODY(job_conjunction_continuation, andands, orors, empty);
 };
 
+/// The time builtin.
+DEF_ALT(optional_time) {
+    using empty = grammar::empty;
+    using time = single<keyword<parse_keyword_time>>;
+    ALT_BODY(optional_time, empty, time);
+};
+
 // A job is a non-empty list of statements, separated by pipes. (Non-empty is useful for cases
 // like if statements, where we require a command). To represent "non-empty", we require a
 // statement, followed by a possibly empty job_continuation, and then optionally a background
 // specifier '&'
 DEF(job)
-produces_sequence<variable_assignments, statement, job_continuation, optional_background>{
-    BODY(job)};
+produces_sequence<optional_time, variable_assignments, statement, job_continuation,
+                  optional_background>{BODY(job)};
 
 DEF_ALT(job_continuation) {
     using piped =
@@ -322,8 +328,9 @@ produces_sequence<keyword<parse_keyword_function>, argument, argument_list, tok_
     BODY(function_header)};
 
 DEF_ALT(not_statement) {
-    using nots = seq<keyword<parse_keyword_not>, variable_assignments, statement>;
-    using exclams = seq<keyword<parse_keyword_exclam>, variable_assignments, statement>;
+    using nots = seq<keyword<parse_keyword_not>, variable_assignments, optional_time, statement>;
+    using exclams =
+        seq<keyword<parse_keyword_exclam>, variable_assignments, optional_time, statement>;
     ALT_BODY(not_statement, nots, exclams);
 };
 
diff --git a/src/parse_grammar_elements.inc b/src/parse_grammar_elements.inc
index 4c88d7f82..343af206e 100644
--- a/src/parse_grammar_elements.inc
+++ b/src/parse_grammar_elements.inc
@@ -31,6 +31,7 @@ ELEM(argument)
 ELEM(redirection)
 ELEM(optional_background)
 ELEM(optional_newlines)
+ELEM(optional_time)
 ELEM(end_command)
 ELEM(freestanding_argument_list)
 #undef ELEM
diff --git a/src/parse_productions.cpp b/src/parse_productions.cpp
index 3f56f7ca6..9584ee096 100644
--- a/src/parse_productions.cpp
+++ b/src/parse_productions.cpp
@@ -82,10 +82,6 @@ RESOLVE(job_decorator) {
             *out_tag = parse_job_decoration_or;
             return production_for<ors>();
         }
-        case parse_keyword_time: {
-            *out_tag = parse_job_decoration_time;
-            return production_for<times>();
-        }
         default: {
             *out_tag = parse_job_decoration_none;
             return production_for<empty>();
@@ -400,6 +396,19 @@ RESOLVE(optional_background) {
     }
 }
 
+RESOLVE(optional_time) {
+    UNUSED(token2);
+
+    switch (token1.keyword) {
+        case parse_keyword_time:
+            *out_tag = parse_optional_time_time;
+            return production_for<time>();
+        default:
+            *out_tag = parse_optional_time_no_time;
+            return production_for<empty>();
+    }
+}
+
 const production_element_t *parse_productions::production_for_token(parse_token_type_t node_type,
                                                                     const parse_token_t &input1,
                                                                     const parse_token_t &input2,
diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp
index fce988a01..0427f48c4 100644
--- a/src/parse_tree.cpp
+++ b/src/parse_tree.cpp
@@ -955,7 +955,7 @@ void parse_ll_t::accept_tokens(parse_token_t token1, parse_token_t token2) {
                     case symbol_job:
                         variable_assignments =
                             tnode_t<grammar::job>(&nodes, parent)
-                                .try_get_child<grammar::variable_assignments, 0>();
+                                .try_get_child<grammar::variable_assignments, 1>();
                         break;
                     case symbol_job_continuation:
                         variable_assignments =
diff --git a/src/proc.h b/src/proc.h
index 341dc028e..ea8ab09ec 100644
--- a/src/proc.h
+++ b/src/proc.h
@@ -421,6 +421,9 @@ class job_t {
 
         /// This job is disowned, and should be removed from the active jobs list.
         bool disown_requested{false};
+
+        /// Whether to print timing for this job.
+        bool has_time_prefix{false};
     } job_flags{};
 
     /// Access the job flags.
diff --git a/src/timer.cpp b/src/timer.cpp
index d4289a766..454696385 100644
--- a/src/timer.cpp
+++ b/src/timer.cpp
@@ -195,3 +195,22 @@ wcstring timer_snapshot_t::print_delta(timer_snapshot_t t1, timer_snapshot_t t2,
 
     return output;
 };
+
+static std::vector<timer_snapshot_t> active_timers;
+
+static void pop_timer() {
+    auto t1 = std::move(active_timers.back());
+    active_timers.pop_back();
+    auto t2 = timer_snapshot_t::take();
+
+    // Well, this is awkward. By defining `time` as a decorator and not a built-in, there's
+    // no associated stream for its output!
+    auto output = timer_snapshot_t::print_delta(std::move(t1), std::move(t2), true);
+    std::fwprintf(stderr, L"%S\n", output.c_str());
+}
+
+cleanup_t push_timer(bool enabled) {
+    if (!enabled) return {[]() {}};
+    active_timers.emplace_back(timer_snapshot_t::take());
+    return {[]() { pop_timer(); }};
+}
diff --git a/src/timer.h b/src/timer.h
index bdc34bc2e..997e98c11 100644
--- a/src/timer.h
+++ b/src/timer.h
@@ -13,6 +13,8 @@
 class parser_t;
 struct io_streams_t;
 
+cleanup_t push_timer(bool enabled);
+
 struct timer_snapshot_t {
    public:
     struct rusage cpu_fish;
diff --git a/src/tnode.cpp b/src/tnode.cpp
index 450098f69..ed64dcda5 100644
--- a/src/tnode.cpp
+++ b/src/tnode.cpp
@@ -114,7 +114,7 @@ arguments_node_list_t get_argument_nodes(tnode_t<grammar::arguments_or_redirecti
 }
 
 bool job_node_is_background(tnode_t<grammar::job> job) {
-    tnode_t<grammar::optional_background> bg = job.child<3>();
+    tnode_t<grammar::optional_background> bg = job.child<4>();
     return bg.tag() == parse_background;
 }
 
@@ -144,7 +144,7 @@ pipeline_position_t get_pipeline_position(tnode_t<grammar::statement> st) {
 
     // Check if we're the beginning of a job, and if so, whether that job
     // has a non-empty continuation.
-    tnode_t<job_continuation> jc = st.try_get_parent<job>().child<2>();
+    tnode_t<job_continuation> jc = st.try_get_parent<job>().child<3>();
     if (jc.try_get_child<statement, 3>()) {
         return pipeline_position_t::first;
     }
diff --git a/tests/checks/time.fish b/tests/checks/time.fish
index dc50d1c4f..38c13dc8e 100644
--- a/tests/checks/time.fish
+++ b/tests/checks/time.fish
@@ -25,3 +25,21 @@ time echo 'foo -s   bar'
 #CHECKERR: Executed in {{[\d,.\s]*}} {{millis|micros|secs}} {{\s*}}fish {{\s*}}external
 #CHECKERR: usr time {{[\d,.\s]*}} {{millis|micros|secs}} {{[\d,.\s]*}} {{millis|micros|secs}} {{[\d,.\s]*}} {{millis|micros|secs}}
 #CHECKERR: sys time {{[\d,.\s]*}} {{millis|micros|secs}} {{[\d,.\s]*}} {{millis|micros|secs}} {{[\d,.\s]*}} {{millis|micros|secs}}
+
+true && time a=b not builtin true | true
+#CHECKERR: ___{{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+
+PATH= time true
+#CHECKERR: fish: Unknown command: time
+#CHECKERR: {{.*}}
+#CHECKERR: PATH= time true
+#CHECKERR:       ^
+
+not time true
+#CHECKERR: ___{{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}