negate: replace special case with parens

v0: negate: add parens for negative values
    Before this patch -(-1) was formatted to --1, but this is not valid
    syntax.
v2: replace special case with parens, otherwise formatting of -(-(-1))
    will be unstable, in v0 it was formatted to -(1) but -(1) is
    formatted to -1 [1].

  [1]: https://clickhouse-test-reports.s3.yandex.net/24427/4512460aeb02fec557c558eee3f8291a6297efb2/fuzzer_asan/fuzzer.log

src/Parsers/ASTFunction.cpp

@@ -231,68 +231,32 @@ void ASTFunction::formatImplWithoutAlias(const FormatSettings & settings, Format
                 }
 
                 const auto * literal = arguments->children[0]->as<ASTLiteral>();
-                /* A particularly stupid case. If we have a unary minus before
+                const auto * function = arguments->children[0]->as<ASTFunction>();
-                 * a literal that is a negative number "-(-1)" or "- -1", this
+                bool negate = name == "negate";
-                 * can not be formatted as `--1`, since this will be
+                // negate always requires parentheses, otherwise -(-1) will be printed as --1
-                 * interpreted as a comment. Instead, negate the literal
+                bool negate_need_parens = negate && (literal || (function && function->name == "negate"));
-                 * in place. Another possible solution is to use parentheses,
-                 * but the old comment said it is impossible, without mentioning
-                 * the reason. We should also negate the nonnegative literals,
-                 * for symmetry. We print the negated value without parentheses,
-                 * because they are not needed around a single literal. Also we
-                 * use formatting from FieldVisitorToString, so that the type is
-                 * preserved (e.g. -0. is printed with trailing period).
-                 */
-                if (literal && name == "negate")
-                {
-                    written = applyVisitor(
-                        [&settings](const auto & value)
-                        // -INT_MAX is negated to -INT_MAX by the negate()
-                        // function, so we can implement this behavior here as
-                        // well. Technically it is an UB to perform such negation
-                        // w/o a cast to unsigned type.
-                        NO_SANITIZE_UNDEFINED
-                        {
-                            using ValueType = std::decay_t<decltype(value)>;
-                            if constexpr (is_decimal_field<ValueType>)
-                            {
-                                // The parser doesn't create decimal literals, but
-                                // they can be produced by constant folding or the
-                                // fuzzer. Decimals are always signed, so no need
-                                // to deduce the result type like we do for ints.
-                                const auto int_value = value.getValue().value;
-                                settings.ostr << FieldVisitorToString{}(ValueType{
-                                    -int_value,
-                                    value.getScale()});
-                            }
-                            else if constexpr (std::is_arithmetic_v<ValueType>)
-                            {
-                                using ResultType = typename NumberTraits::ResultOfNegate<ValueType>::Type;
-                                settings.ostr << FieldVisitorToString{}(
-                                    -static_cast<ResultType>(value));
-                                return true;
-                            }
-
-                            return false;
-                        },
-                        literal->value);
-
-                    if (written)
-                    {
-                        break;
-                    }
-                }
-
                 // We don't need parentheses around a single literal.
-                if (!literal && frame.need_parens)
+                bool need_parens = !literal && frame.need_parens && !negate_need_parens;
+
+                // do not add extra parentheses for functions inside negate, i.e. -(-toUInt64(-(1)))
+                if (negate_need_parens)
+                    nested_need_parens.need_parens = false;
+
+                if (need_parens)
                     settings.ostr << '(';
 
                 settings.ostr << (settings.hilite ? hilite_operator : "") << func[1] << (settings.hilite ? hilite_none : "");
 
+                if (negate_need_parens)
+                    settings.ostr << '(';
+
                 arguments->formatImpl(settings, state, nested_need_parens);
                 written = true;
 
-                if (!literal && frame.need_parens)
+                if (negate_need_parens)
+                    settings.ostr << ')';
+
+                if (need_parens)
                     settings.ostr << ')';
 
                 break;

tests/queries/0_stateless/01881_negate_formatting.reference

@@ -0,0 +1,13 @@
+-- { echo }
+EXPLAIN SYNTAX SELECT -1;
+SELECT -1
+EXPLAIN SYNTAX SELECT -(1);
+SELECT -(1)
+EXPLAIN SYNTAX SELECT -(-(1));
+SELECT -(-(1))
+EXPLAIN SYNTAX SELECT -(-(-(1)));
+SELECT -(-(-(1)))
+EXPLAIN SYNTAX SELECT -(-(-1));
+SELECT -(-(-1))
+EXPLAIN SYNTAX SELECT -(-toUInt64(-(1)));
+SELECT -(-toUInt64(-(1)))

tests/queries/0_stateless/01881_negate_formatting.sql

@@ -0,0 +1,7 @@
+-- { echo }
+EXPLAIN SYNTAX SELECT -1;
+EXPLAIN SYNTAX SELECT -(1);
+EXPLAIN SYNTAX SELECT -(-(1));
+EXPLAIN SYNTAX SELECT -(-(-(1)));
+EXPLAIN SYNTAX SELECT -(-(-1));
+EXPLAIN SYNTAX SELECT -(-toUInt64(-(1)));