Using 'function' in a Lua string causes wrong indentation

[GniLudio]

Does this issue occur when all extensions are disabled?: Yes

• Version: 1.84.2 (user setup)
• Commit: 1a5daa3
• Date: 2023-11-09T10:51:52.184Z
• Electron: 25.9.2
• ElectronBuildId: 24603566
• Chromium: 114.0.5735.289
• Node.js: 18.15.0
• V8: 11.4.183.29-electron.0
• OS: Windows_NT x64 10.0.23481

Steps to Reproduce:

1. Create a Lua file
2. Create a string which includes function
   • there must be no letter before and after function
3. Add a newline after the string

[tomlau10]

Hi, I just ran into this issue recently with code snippet like this:

function service.addCmd(name, func)
    assert(type(func) == "function", name .. " is not function")
        |<<< undesired indent here

This seems to be caused by the imperfect increaseIndentPattern here:

vscode/extensions/lua/language-configuration.json

Lines 25 to 26 in 1c7fa5e

	"indentationRules": {
	"increaseIndentPattern": "^((?!(\\-\\-)).)*((\\b(else|function|then|do|repeat)\\b((?!\\b(end|until)\\b).)*)|(\\{\\s*))$",

I would say the current pattern already solves 9x% of normal use cases 👍 but it has a few major flaws:

1. when keywords are just in a string literal, it will cause false positive cases, just like this issue and my example
2. when the end|until is in trailing comment, it will cause false negative cases, like the following case

if true then -- not the end
|<<< missing indent
-- this is because the word `end` is matched after `then`

3. if a -- (which normally means a comment) is in a string literal, it will cause false negative as well:

if s == "---" then
|<<< missing indent
-- that's because nothing will be matched after `--` is encountered

---

I have been trying to improve the pattern and I think I have come up with a better solution 🚀 (as well as some tests to assert its correctness).
I noticed that this issue has already been assigned to @aiday-mar , so is it is fine for me to directly create a PR and link to this issue then discuss there? Or should I post my solution here and discuss first before I open a PR? Thanks 😄

[GniLudio]

Hi, I just ran into this issue recently with code snippet like this:

function service.addCmd(name, func)
    assert(type(func) == "function", name .. " is not function")
        |<<< undesired indent here

This seems to be caused by the imperfect increaseIndentPattern here:

vscode/extensions/lua/language-configuration.json

Lines 25 to 26 in 1c7fa5e

	"indentationRules": {
	"increaseIndentPattern": "^((?!(\\-\\-)).)*((\\b(else|function|then|do|repeat)\\b((?!\\b(end|until)\\b).)*)|(\\{\\s*))$",

I would say the current pattern already solves 9x% of normal use cases 👍 but it has a few major flaws:

1. when keywords are just in a **string literal**, it will cause **false positive** cases, just like this issue and my example

2. when the `end|until` is in **trailing comment**, it will cause **false negative** cases, like the following case

if true then -- not the end
|<<< missing indent
-- this is because the word `end` is matched after `then`

3. if a `--` (which normally means a comment) is in a **string literal**, it will cause **false negative** as well:

if s == "---" then
|<<< missing indent
-- that's because nothing will be matched after `--` is encountered

I have been trying to improve the pattern and I think I have come up with a better solution 🚀 (as well as some tests to assert its correctness). I noticed that this issue has already been assigned to @aiday-mar , so is it is fine for me to directly create a PR and link to this issue then discuss there? Or should I post my solution here and discuss first before I open a PR? Thanks 😄

Is there a reason not to post your solution here before creating a PR?

[tomlau10]

It's totally fine to post it here 😄 Here is my current solution and I will explain it below.
(I shall attach my test.js tomorrow as it is saved in my office computer, and I don't have it right now)

"increaseIndentPattern": "^((?!\\-\\-)[^\"']|\"(\\\\\"|(?!\\\\\")[^\"])*\"|'(\\\\'|(?!\\\\')[^'])*')*(\\b(else|then|do|repeat)\\b|\\bfunction\\b([^\\)]+\\)?)?|[\\{\\(])\\s*(\\-\\-.*)?$",

• The unescaped form which can directly be copy pasted to site like regex101: https://regex101.com/

^((?!\-\-)[^"']|"(\\"|(?!\\")[^"])*"|'(\\'|(?!\\')[^'])*')*(\b(else|then|do|repeat)\b|\bfunction\b([^\)]+\)?)?|[\{\(])\s*(\-\-.*)?$

Explanation

As mentioned, the existing pattern has 3 major flaws, and my solution aims to fix all of them

1. keywords in string literal

To properly ignore keywords in string literal, the idea is to consume the quoted string when matching the sequence before a keyword. I borrowed the idea from this post: https://stackoverflow.com/a/23667311

• For simplicity, now just assume that string will only be quoted by double quotes "string literal"
  Instead of just using dot . to match any character in the prefix sequence, we break it into 2 cases: (non quote character | quote character)
  • if it's a non quote character, we simply match it: [^"]
    otherwise we should consume then entire quoted string, i.e. match until the closing quote: "[^"]*"
    this gives something like: ([^"]|"[^"]*")*
  • in order to handle escaped quote, the [^"]* part needs to break up into another 2 cases:
    • the escaped quote sequence \\"
    • the any character except quote, as well as not the escaped quote sequence (?!\\")[^"]
    • together [^"]* becomes (\\"|[^"])*
      => so this part finally becomes ([^"]|"(\\"|(?!\\")[^"])*")*
• Now lua strings can also be quoted using single quote 'string literal
  They are the same, just replace double quote with single quote: ([^']|'(\\'|(?!\\')[^'])*')*
  After merging with the above double quote case: ([^"']|"(\\"|(?!\\")[^"])*"|'(\\'|(?!\\')[^'])*')*
• We need to keep the ability to skip comments --, so add back the (?!\-\-) at the beginning
  => ((?!\-\-)[^"']|"(\\"|(?!\\")[^"])*"|'(\\'|(?!\\')[^'])*')*

This is the first part of the solution

2. keywords in trailing comment

The above trick guarantees that the matched keywords are not part of a string. Now we have to deal with the case that in 1 liner, if having the end|until keyword, the next line should not be indented. Just checking end|until is not enough, as they may be in the middle of a string or trailing comment.

• It's very troublesome to this checking, so instead I took a totally different approach: only allow trailing spaces or comment after the matched keywords
  => \\s*(\\-\\-.*)?
• Note that this is a much stricter condition for the regex to match, but I think it holds true most of the time 😕 Let's consider each keywords in the regex
  • else|then|do|repeat
    we either write it as else a = a + 1 end / then a = a + 1 end
    or just the keyword else / then and write the statement next line
    I don't think there will be a case to write it as

  else a = a + 1
    b = b + 1
  end
  -- the above style is nonsense

  • function
    This keyword is much trickier, because a function can be named or anonymous, and it maybe followed by a (non finished) param list. My observation is that in valid syntax, if we expect an indentation, then the function declaration at most should end with a closing bracket ). This closing bracket is optional, but once we matched it, nothing (except trailing spaces and comment of course) should be allowed

  -- should indent
  function ()
  function a(p1)
  function a(p1, p2, ...)
  function a(p1,

  -- should not indent
  function a(a1) a = a + 1 end
  
  -- will not have indent, and it makes no sense to write this
  function a(a1) a = a + 1
  end

  So it doesn't matter whether we check for not followed by end. It should be enough to just allow trailing spaces or comment in order to decide if next line should be indented.
• We need to achieve function keyword optionally followed by anything, and if there exist anything we should stop if ) is matched
  • [^)]+: the anything (except ) of course)
  • [^)]+\)?: followed by the optional )
  • ([^)]+\)?)?: this whole part should be optional
  • \bfunction\b([^\)]+\)?)?: all together
• The above should be an alternative match to \b(else|then|do|repeat)\b, so they are joined using a |
  => \b(else|then|do|repeat)\b|\bfunction\b([^\)]+\)?)?

This is the 2nd part of the solution.

3. -- in a string would cause false negative

This is solved by the 1st part of the solution, as strings are now properly ignored. And any match for -- is really a comment prefix now

bonus: allow trailing comments after (|{

In the original pattern \{\s*, it just allow trailing spaces after {

• To allow ( as well, \{ is changed to [\{\(]
• And to allow trailing comment, I moved this part to join the above keywords group
  => \b(else|then|do|repeat)\b|\bfunction\b([^\)]+\)?)?|[\{\(]
• And finally this whole group is followed by trailing spaces and optional comment
  => (\b(else|then|do|repeat)\b|\bfunction\b([^\)]+\)?)?|[\{\(])\s*(\-\-.*)?$

All together 🎉

^((?!\-\-)[^"']|"(\\"|(?!\\")[^"])*"|'(\\'|(?!\\')[^'])*')*(\b(else|then|do|repeat)\b|\bfunction\b([^\)]+\)?)?|[\{\(])\s*(\-\-.*)?$

Cavet

• Note that multi lined string [[some string]] is not handled here. It would be too troublesome as its syntax can have as many = between the [[

local s = [==[
this is a multi line string
which can use [[ ]] inside it
]==]

• Another reason is that it seems I cannot handle it 😕
  My regex above can detect correctly that: if s == [[then]] should NOT be indented (which can be verified in regex101)
  However in vscode, the next line is still indented.
  I tried to use developer tool of vscode to debug it, and found that the actual string being tested against is if s == then
  => somehow the [[/]] are stripped before passing to the auto indent regex test module

---

I hope the explanation above is clear enough. 🙂
And as mentioned, I shall post my test.js tomorrow.
Meanwhile you may test it via regex101, or edit the extensions/lua/language-configuration.json file locally to test it directly in vscode

[tomlau10]

I just found a much efficient way to check for quoted string 👀
"[^"\\]*(?:\\.[^"\\]*)*"
ref: https://stackoverflow.com/a/6525975

I compared its performance using the debugger in regex101, and its much faster (use much fewer steps) than my proposed "(\\"|(?!\\")[^"])*"

• test string: local s = "this is a test string"
  • my solution: 79 steps
  • found solution: 16 steps
• test string: local s = "this is a test string
  • my solution: 121 steps to report failed match
  • found solution: 37 steps to report failed match

---

I shall definitely incorporate it into my proposed solution 😄

[tomlau10]

Test file

Here is my test.js, and it includes the different stages of the regex during the development of my solution. You may change the const PATTERN = to point to different stages to see how the test results evolves.

// original
const stage0 = /^((?!(\-\-)).)*((\b(else|function|then|do|repeat)\b((?!\b(end|until)\b).)*)|(\{\s*))$/;

// remove unnecessary brackets
const stage1 = /^((?!\-\-).)*(\b(else|function|then|do|repeat)\b((?!\b(end|until)\b).)*|\{\s*)$/;

// skip string before keywords
const stage2 = /^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b((?!\b(end|until)\b).)*|\{\s*)$/;

// allow only trailing spaces or comment after keyword
const stage3 = /^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b\s*(\-\-.*)?|\{\s*)$/;

// fix allow param list after function keyword
const stage4 = /^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b((?<=function)[^)]+\)?)?\s*(\-\-.*)?|\{\s*)$/;

// support ending with "("
const stage5 = /^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b((?<=function)[^)]+\)?)?\s*(\-\-.*)?|[\{\(]\s*)$/;

// allow trailing comment after `{` or `(`
const stage6 = /^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b((?<=function)[^)]+\)?)?|[\{\(])\s*(\-\-.*)?$/;

// annotated
const stage7 = /^(?<not_cmt>(?!\-\-)[^"']|(?<dq_str>"[^"\\]*(\\.[^"\\]*)*")|(?<sq_str>'[^'\\]*(\\.[^'\\]*)*'))*(\b(?<kw>else|function|then|do|repeat)\b(?<fparam>(?<=function)[^)]+\)?)?|(?<bracket>[\{\(]))(?<trail>\s*(\-\-.*)?)$/;

// pattern to test
const PATTERN = stage7;
console.log(`PATTERN = ${PATTERN}`);
console.log(`escapsed = ${JSON.stringify(PATTERN.toString().slice(1,-1))}`);

// simple test framework
function test(id, s, expect) {
    if (PATTERN.test(s) !== expect) {
        console.log(`#${id} fail: ${s}`);
    }
}

const positive = String.raw `
if true then
if a == 1 then -- until now
then
else
if true then -- not the end
if s == "--" then
if s == "then" then
if s == "\"then\"" then
if s == "\"--\"" then
if s == '\'--\'' then

function
function a()
local function a()
local a = function ()
function a(p1)
local function a(p1, p2)
local a = function (p1, p2, ...)
table.sort(function (a, b)
function (
function a(
function a(p1, p2,
function a.b(p1, ...)
function -- (
function -- )
function -- ) end

do
while true do
while true do -- end
repeat
repeat -- until

do -- end
{
t = {
local t = {
local t = { -- comment

func(
a = func(
a = func( -- not the end
`

const negative = String.raw `
if
elseif
then end
else end
-- if true then
if s == "then"
if s == "\"then"
if true then end

function () end
function a() end
local function a() end
local a = function() end
local function1 = nil

do end
while true do end
repeat until
repeat until true
do end
do s = "--" end

local a = 1 -- do
local a = -1 -- then
local s = "then"
local s = [[then]]
print("then")
print("function")
assert(type(func) == "function")
`

console.log("===== testing positive cases =====")
positive.split("\n").filter(line => line).forEach((s, i) => test(i+1, s, true));

console.log("===== testing negative cases =====")
negative.split("\n").filter(line => line).forEach((s, i) => test(i+1, s, false));

Latest solution

The latest solution is slightly changed compare with my previous version. I used positive lookbehind to check for function params and put it after the keywords branch: ((?<=function)[^)]+\)?)?. I think that would make it more readable and editable, as all the keywords are grouped together. 🤔

^((?!\-\-)[^"']|"[^"\\]*(\\.[^"\\]*)*"|'[^'\\]*(\\.[^'\\]*)*')*(\b(else|function|then|do|repeat)\b((?<=function)[^)]+\)?)?|[\{\(])\s*(\-\-.*)?$

Annotated version

I have as well made an annotated version (using named group) for more verbosity:

^(?<not_cmt>(?!\-\-)[^"']|(?<dq_str>"[^"\\]*(\\.[^"\\]*)*")|(?<sq_str>'[^'\\]*(\\.[^'\\]*)*'))*(\b(?<kw>else|function|then|do|repeat)\b(?<fparam>(?<=function)[^)]+\)?)?|(?<bracket>[\{\(]))(?<trail>\s*(\-\-.*)?)$

Here is the breakdown again

• ^(?<not_cmt>(?!\-\-)[^"']|(?<dq_str>"[^"\\]*(\\.[^"\\]*)*")|(?<sq_str>'[^'\\]*(\\.[^'\\]*)*'))*: match until comment, consume entire quoted string when encountered
  • (?!\-\-)[^"']: any character not comment and not quotes, mutually exclusive in the group
  • (?<dq_str>"[^"\\]*(\\.[^"\\]*)*"): match double quoted string
  • (?<sq_str>'[^'\\]*(\\.[^'\\]*)*'): match single quoted string
• (\b(?<kw>else|function|then|do|repeat)\b(?<fparam>(?<=function)[^)]+\)?)?|(?<bracket>[\{\(])): match keywords or brackets
  • \b(?<kw>else|function|then|do|repeat)\b: match keywords at boundary
    • (?<fparam>(?<=function)[^)]+\)?)?: match optional param list specific to function keyword
  • (?<bracket>[\{\(]): match brackets
• (?<trail>\s*(\-\-.*)?)$: if anything after keywords or brackets, must be trailing spaces or comment