Wrap inline negative const enum with parens

[Andarist]

supersedes #55031
fixes #54992

cc @a-tarasyuk @jakebailey

[Andarist]

This PR is not fully ready - it requires just small tweaks. I would like to get the TS team opinion as to what is preferred first.

I think that the whole double dot thing could likely just be replaced with parens (I think that @jakebailey might agree with me on this one: #55031 (comment) ). I could just add parenthesis for negative numbers and double dot for positive ones but honestly, that just sounds like a redundant complication to me.

[jakebailey]

I'm torn; I think that adding these parens is a little overzealous; my main comparison is esbuild which instead inserts a space before the dot, which is roughly equivalent to a double dot, but it seems like there's no particular consistency in the ecosystem besides double-dot or space being one character fewer for the same thing.

Of course, esbuild always includes the comment too unless you specify minify=true (the highest minification), and tsc isn't a minifier, so it's not like it's ridiculous to work differently. esbuild will also convert (1).toString() to 1 .toString() so who knows.

Example: esbuild playground

[rbuckton]

I'd rather stick with the .. emit wherever possible and only parenthesize when it is absolutely necessary, i.e., when the number itself contains - or if the left-hand-side of a property access/element access is a prefix-unary +/- whose operand is a numeric literal.

[rbuckton]

The simplest fix would be to:

1. Parenthesize negative numeric literals in parenthesizeLeftSideOfAccess

2. Modify emitPropertyAccessExpression to capture the result of parenthesization, e.g., replace the first line of the function with

   let expression = node.expression;
   emitExpression(expression, node => expression = parenthesizer.parenthesizeLeftSideOfAccess(node));

   and replace the shouldEmitDotDot declaration with

   const shouldEmitDotDot =
     token.kind !== SyntaxKind.QuestionDotToken &&
     mayNeedDotDotForPropertyAccess(expression) &&
     !writer.hasTrailingComment() &&
     !writer.hasTrailingWhitespace();

[rbuckton]

Actually, there is a better fix. The only place we really produce negative numbers for constant inlining is in substituteConstantValue in the ts transform. We should instead fix that to turn a numeric value of -1 into a PrefixUnaryExpression for a numeric literal (and explicitly parenthesizing).

[jakebailey]

Probably @rbuckton has the most well-formed opinion here.

[rbuckton]

@Andarist: I can put up a PR for this that introduces parens in far fewer places, but if you'd rather do it the simplest approach would be to fix substituteConstantValue to avoid returning negative numeric literals and instead return parenthesized, prefix unary wrapped numeric literals.

We can reduce unnecessary parentheses even more if don't always parenthesize the prefix unary in substituteConstantValue and we instead modify substitutePropertyAccessExpression and substituteElementAccessExpression to run substitutions early on the node's expression so that we can trigger the parenthesizer during substitution. Something like this:

function substitutePropertyAccessExpression(node: PropertyAccessExpression) {
    if (isAccessExpression(node.expression)) {
        node = isPropertyAccessChain(node) ?
            factory.updatePropertyAccessChain(node, substituteExpression(node.expression), node.questionDotToken, node.name):
            factory.updatePropertyAccessExpression(node, substituteExpression(node.expression), node.name);
    }
    return substituteConstantValue(node);
}

Note that the second approach doesn't cover other MemberExpressions like NewExpression or TaggedTemplateExpression. We could consider supporting those cases, though you're not likely to see new E.Foo() or E.Foo`bar` (where E is an enum) in the wild and it not already be an error.

[Andarist]

@rbuckton i specifically didnt fix this while doing const substitution because that felt like a targeted fix. I can imagine that a custom TS transformer could do a similar substitution and then it wouldnt benefit from the fix.

[Andarist]

@Andarist: I can put up a PR for this that introduces parens in far fewer places, but if you'd rather do it

Usually, I prefer to do changes on my own. It's always a good learning experience 😉

@rbuckton from what I can tell, this is the most recent suggestion now, right?

[jakebailey]

After the most recent changes, the output looks good to me.

[sandersn]

@jakebailey Do you want to wait for @rbuckton to sign off too, or is it good to go now?

[jakebailey]

As far as I can tell, this PR matches what Ron suggested ("fix substituteConstantValue to avoid returning negative numeric literals and instead return parenthesized, prefix unary wrapped numeric literals"), so, I'd say so.