Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tall-style] Keep qualified names tighter than the containing expression #1557

Open
rakudrama opened this issue Sep 4, 2024 · 1 comment

Comments

@rakudrama
Copy link
Member

Here the first js is a library import prefix. It should not IMO be separated from the thing it qualifies.
(The formatting might be reasonable if the first js was a variable).

    return js.ExpressionStatement(
      js
          .js('# = #', [goto, js.number(label)])
          .withSourceInformation(sourceInformation),
    );

This would be better:

    return js.ExpressionStatement(
        js.js('# = #', [goto, js.number(label)])
        .withSourceInformation(sourceInformation),
    );

See https://dart-review.googlesource.com/c/sdk/+/383722/1/pkg/compiler/lib/src/js/rewrite_async.dart#315

@rakudrama rakudrama changed the title Keep qualified names tighter than the containing expression [tall-style] Keep qualified names tighter than the containing expression Sep 4, 2024
@munificent
Copy link
Member

I agree that the output here doesn't look great. That's often the case when you have a split call chain and the target (here just js) is shorter than 4 characters. It ends up looking like it's hanging out in space.

It's particularly egregious here because this isn't really a call chain with two method calls. It's a prefixed bare function call followed by a method call. Ideally, it would get formatted something like:

    return js.ExpressionStatement(
      js.js('# = #', [goto, js.number(label)])
          .withSourceInformation(sourceInformation),
    );

However, the formatter doesn't resolve anything. It doesn't know that js is a prefix and not an identifier expression.

You might argue that the formatter could know that because the prefixes are always easily textually found at the top of the file. However, that's not true when:

  • We are formatting a part file and the imports are in some other file. (Enhanced part files will make this worse.)
  • You are formatting a chunk of code which you will then paste into some other file.
  • The prefix is shadowed by some other local declaration.

Sometimes I wish we used a different separator for prefixes, like ::. But the formatter has to deal with what it's given and format with very little context.

There is one hacky heuristic the formatter uses to guess at what a name means. It uses capitalization to guess that an identifier is a type name versus a function name. That shows up in things like named constructors and static methods. If you have Iterable.generate(), the formatter won't split on that . because it sees that the first identifier is capitalized. Likewise, if you have name.SomeClass(), it assumes name is a prefix and not an identifier expression because of the subsequent capitalized name. Therefore it won't split after name.

However, that doesn't help here because the thing you're calling with a prefix is a top level function whose name is lowercase. :( If it was js.Js, then the formatter would give you:

    return js.ExpressionStatement(
      js.Js('# = #', [
        goto,
        js.number(label),
      ]).withSourceInformation(sourceInformation),
    );

But of course, you don't want to give a function a capitalized name.

So, I agree the output doesn't look great here, but it's probably about the best we can do. Sorry I don't have a great solution. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants