Proposal for an alternative one-item-per-line linting of set literal expressions

[gagliardetto]

A lot of people write codeql queries. Many of them get to a certain point of their journey where they need to match a variable to a set of possible values.

Currently, the most readable way to do that looks something like this (after linting):

    exists(string val |
      val = "aaaaaaaaaaaaaaaaaa" or
      val = "bbbbbbbb" or
      val = "ccccccccccc" or
      val = "dddddddd" or
      val = "eeeeeeeeeeeee" or
      val = "fffffffffffffff" or
      val = "gggggggggg" or
      val = "hhhhhhhhhhh" or
      val = "iiiiiiiiiiiiiiiiii" or
      val = "jjjjjjjj"
    )

That's quite readable, but somewhat repetitive, and not very clean.

An alternative to using a lot of or val = is a set literal expression.

Here's what the above snippet looks like re-written using a set literal (after linting):

    exists(string val |
      val in
        ["aaaaaaaaaaaaaaaaaa", "bbbbbbbb", "ccccccccccc", "dddddddd", "eeeeeeeeeeeee",
            "fffffffffffffff", "gggggggggg", "hhhhhhhhhhh", "iiiiiiiiiiiiiiiiii", "jjjjjjjj"]
    )

My proposal is to lint that into this:

    exists(string val |
      val in [
                "aaaaaaaaaaaaaaaaaa",
                "bbbbbbbb",
                "ccccccccccc",
                "dddddddd",
                "eeeeeeeeeeeee",
                "fffffffffffffff",
                "gggggggggg",
                "hhhhhhhhhhh",
                "iiiiiiiiiiiiiiiiii",
                "jjjjjjjj"
          ]
    )

Having one item per line makes the sets a lot more readable, and easier to copy-paste and move around.

Of course we don't have to force users to one-item-per-line formatting. But if they write one item per line, that should be respected and maintained by the linter.

E.g.

If the user writes

    exists(string val |
      val in
        ["a", "b", "b", "c", "d"]
    )

... then that should stay onlined.

But if the user writes

    exists(string val |
      val in
        [
          "a",
          "b",
          "b",
          "c",
          "d"
        ]
    )

... then that should be accepted as valid, too, and the one-per-line logic shall be maintained by the linter.

[jbj]

A desire for each item in the list to stand out like that often means the reader should be aware of something special for each line. When that something is written in an end-of-line comment, the autoformatter preserves the line breaks. It looks like this: https://github.com/github/codeql/blob/92494441a72cd1c04c9359dc8c0d35e2a4525f6d/cpp/ql/src/semmle/code/cpp/commons/Strcat.qll#L12-L18. The indentation is not quite perfect, but we get by.

Would readers also benefit from comments in your use case?

[gagliardetto]

Hey @jbj

Thanks for your answer! This will quite work for my use case. And yes, each item had a comment anyway.

[gagliardetto]

Note; the linter does this:

If I write this:

    exists(string val |
      val in [// before
            "a", // after
            "b", // this is a comment
            "b", // this is a comment
            "c", // this is a comment
            "d"] // this is a comment
    )

... then the linter does this:

    exists(string val |
      val in ["a",
            // before
            // after
            "b", // this is a comment
            "b", // this is a comment
            "c", // this is a comment
            "d"] // this is a comment
    )

---

If this:

    exists(string val |
      val in ["a",// this is a comment
            "b", // this is a comment
            "b", // this is a comment
            "c", // this is a comment
            "d" // before
          ] // after
    )

... then this happens:

    exists(string val |
      val in ["a", // this is a comment
            "b", // this is a comment
            "b", // this is a comment
            "c", // this is a comment
            "d"] // before // after
    )

[jbj]

Thanks for reporting this. I've found an existing internal issue about this bug, and I've added a link to your explanation in that issue.

[jbj]

@gagliardetto This issue should now be fixed in the autoformatter. It'll be part of the next release of codeql-cli-binaries.

[gagliardetto]

Thank you @jbj !

[smowton]

I imagine the linter authors strongly value there being One Correct Formatting for any given document, rather than several acceptable solutions? If that's the case, we could also consider using vertical alignment to make this clearer--

exists(string val |
      val in
        ["aaaaaaaaaaaaaaaaaa", "bbbbbbbb",      "ccccccccccc", 
         "dddddddd",           "eeeeeeeeeeeee", "fffffffffffffff", 
         "gggggggggg",         "hhhhhhhhhhh",   "iiiiiiiiiiiiiiiiii", 
         "jjjjjjjj"]
    )

[jbj]

It's a consistent principle in the autoformatter that we don't do vertical alignment or other kinds of "indentation as an art form". Whether you agree or not with the reasons for this, I hope we can agree that set literals are not important enough to motivate breaking this principle.