From 92426542311aed257b3226e90f3820d9325ad69e Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 23 May 2021 16:34:31 +0300 Subject: [PATCH] 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 | 72 +++++-------------- .../01881_negate_formatting.reference | 13 ++++ .../0_stateless/01881_negate_formatting.sql | 7 ++ 3 files changed, 38 insertions(+), 54 deletions(-) create mode 100644 tests/queries/0_stateless/01881_negate_formatting.reference create mode 100644 tests/queries/0_stateless/01881_negate_formatting.sql diff --git a/src/Parsers/ASTFunction.cpp b/src/Parsers/ASTFunction.cpp index e8c3775d187..6931008c775 100644 --- a/src/Parsers/ASTFunction.cpp +++ b/src/Parsers/ASTFunction.cpp @@ -231,68 +231,32 @@ void ASTFunction::formatImplWithoutAlias(const FormatSettings & settings, Format } const auto * literal = arguments->children[0]->as(); - /* A particularly stupid case. If we have a unary minus before - * a literal that is a negative number "-(-1)" or "- -1", this - * can not be formatted as `--1`, since this will be - * interpreted as a comment. Instead, negate the literal - * 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; - if constexpr (is_decimal_field) - { - // 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) - { - using ResultType = typename NumberTraits::ResultOfNegate::Type; - settings.ostr << FieldVisitorToString{}( - -static_cast(value)); - return true; - } - - return false; - }, - literal->value); - - if (written) - { - break; - } - } - + const auto * function = arguments->children[0]->as(); + bool negate = name == "negate"; + // negate always requires parentheses, otherwise -(-1) will be printed as --1 + bool negate_need_parens = negate && (literal || (function && function->name == "negate")); // 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; diff --git a/tests/queries/0_stateless/01881_negate_formatting.reference b/tests/queries/0_stateless/01881_negate_formatting.reference new file mode 100644 index 00000000000..a8f6a0c4e66 --- /dev/null +++ b/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))) diff --git a/tests/queries/0_stateless/01881_negate_formatting.sql b/tests/queries/0_stateless/01881_negate_formatting.sql new file mode 100644 index 00000000000..c7e51687654 --- /dev/null +++ b/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)));