From 0275c5e803f72600816ae81d8c590cf79c93e10f Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Thu, 21 Nov 2024 11:02:34 +0100
Subject: [PATCH] Swap variable overrides and time in not statement

This is allowed

	time a=b echo 123

but -- due to an oversight in 3de95038b0 (Make "time" a job prefix,
2019-12-21) -- this is not allowed:

	not time a=b echo 123

Instead, this one one works:

	not a=b time echo 123

which is weird because without the "not" this would run "/bin/time".

It seems wrong that "not" is not like the others. Swap the order
for consistency.

Note that unlike "not", "time" currently needs to come before variable
assignments, so "a=b time true" is disallowed. This matches zsh. POSIX
shells call "/bin/time" here. Since it's ambiguous, erroring out seems
fine. It's weird that we're inconsistent with not here but I guess
"command not" is not expected to have subtly different behavior.

Closes #10890
---
 src/ast.rs             |  4 ++--
 tests/checks/time.fish | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/ast.rs b/src/ast.rs
index 5260ee5eb..04654f908 100644
--- a/src/ast.rs
+++ b/src/ast.rs
@@ -1644,16 +1644,16 @@ pub struct NotStatement {
     parent: Option<*const dyn Node>,
     /// Keyword, either not or exclam.
     pub kw: KeywordNot,
-    pub variables: VariableAssignmentList,
     pub time: Option<KeywordTime>,
+    pub variables: VariableAssignmentList,
     pub contents: Statement,
 }
 implement_node!(NotStatement, branch, not_statement);
 implement_acceptor_for_branch!(
     NotStatement,
     (kw: (KeywordNot)),
-    (variables: (VariableAssignmentList)),
     (time: (Option<KeywordTime>)),
+    (variables: (VariableAssignmentList)),
     (contents: (Statement)),
 );
 impl ConcreteNode for NotStatement {
diff --git a/tests/checks/time.fish b/tests/checks/time.fish
index f4fe070e1..587ab98fe 100644
--- a/tests/checks/time.fish
+++ b/tests/checks/time.fish
@@ -38,6 +38,22 @@ not time true
 #CHECKERR: {{.*}}
 #CHECKERR: {{.*}}
 
+not time a=b true
+#CHECKERR: ___{{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+
+# Currently illegal syntax. Same in zsh. POSIX shells call the external command "time" here.
+a=b time true
+#CHECKERR: fish: time: missing man page
+#CHECKERR: Documentation may not be installed.
+#CHECKERR: `help time` will show an online version
+not a=b time true
+#CHECKERR: fish: time: missing man page
+#CHECKERR: Documentation may not be installed.
+#CHECKERR: `help time` will show an online version
+
 $fish -c 'time true&'
 #CHECKERR: fish: {{.*}}
 #CHECKERR: time true&