Check for onEnterRules that decrease indentation when determining new line indent

[ssigwart]

This PR fixes #136592. However, the single line if fix only works after #136577 is merged.

To test, you can use this:

./scripts/test.sh --glob **/cursor.test.js --grep 136592

[ssigwart]

I rebased this to fix a merge conflict.

[ssigwart]

@hediet, this fixes the following PHP code. If you hit Enter on the last blank line, it will add a space in front of the new line.

/**
 * PHPDoc
 */

Similarly, if you have the following code and hit Enter on the blank line, it will add a tab in front of the new line. If #136577 is merged, this PR will solve this issue too.

if (1)
	1;

This issue is also a problem with other languages that allow single line conditionals. I think #136577 is much more impactful than this PR. This PR is also riskier than that one. I think it's the correct fix, but don't want to rush this one out if it needs more review time.

Thanks!

[lionel-]

@aiday-mar I've noticed that you self-assigned indentation-related opened issues. Could you also please take a look at this PR if you get a chance?

Currently dedention rules are non-sticky and we notice lots of cases like:

foo &&
    bar<CURSOR>

Where enter correctly dedents the first time:

foo &&
    bar
<CURSOR>

But on subsequent times enter suddenly reindents:

foo &&
    bar

    <CURSOR>

[aiday-mar]

Hi @lionel- thank you for pinging on this issue. For the example you gave I believe you could specify a new regex pattern in the indentNextLinePattern of the language configuration file, which will specify that after the line ending with bar indentation should be applied only on the next line. What language are you currently working with?

[lionel-]

Hi @aiday-mar, thanks for looking at this. I'm working on an improved language configuration for the R language. One important use case for R is pipelines of the form:

# ggplot2 pipeline
mtcars |>
  ggplot(aes(
    disp,
    drat
  )) +
  geom_point(
    aes(colour = cyl)
  )

# dplyr pipeline
starwars |>
  group_by(species) |>
  summarise(
    n = n(),
    mass = mean(mass, na.rm = TRUE)
  ) |>
  filter(
    n > 1,
    mass > 50
  )

We have not been able to find a way to correctly indent this kind of pipelines with components spread over multiple lines, which is partly why I opened #208985. However it should at least be possible to indent one-liners pipelines such as:

starwars |>
  group_by(species) |>
  summarise( n = n(), mass = mean(mass, na.rm = TRUE)) |>
  filter(n > 1, mass > 50)

Regarding your idea of using indentNextLinePattern, this will not work in our case. We can't use an increase-indent pattern for lines ending with an operator here, even one that only works on the next line as you suggested, because they produce this kind of staircase indentation:

increase-indent-rule.mov

To avoid this, we are using OnEnter rules that increase indentation when entering a chain of operators and decrease it when the chain is finished:

	"onEnterRules": [
		// A line ending with an operator, preceded by a blank line.
		{
			"previousLineText": "^\\s*(?:#|$)",
			"beforeText":		"^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"action":			{ "indent": "indent" }
    },

		// A line ending with an operator, preceded by a line not ending with an operator.
		{
			"previousLineText": "^(?!.*[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$))[^#]+.*$",
			"beforeText":		"^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"action":			{ "indent": "indent" }
		},

		// A line not ending with an operator, preceded by a line ending with an operator.
		{
			"previousLineText": "^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"beforeText":		"^(?!.*[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$))[^#]+.*$",
			"action":			{ "indent": "outdent" }
		},
	]

With these rules we get the correct indenting behaviour:

onenter-rule.mov

Unfortunately the "outdent" action is not sticky, causing the following indentation bug on ulterior lines:

outdent-stickiness-bug.mov

We have verified that the PR fixes the non-stickiness of OnEnter outdent rules for our use case.

[lionel-]

Another case of this occurs in the javascript extension. Type Enter with this snippet:

while (true)
    foo;<CURSOR>

You end up here:

while (true)
    foo;
<CURSOR>

Type enter again and you get:

while (true)
    foo;

    <CURSOR>

[aiday-mar]

Hi @lionel- thanks for pinging me on this PR. Before merging this PR, I need to look more closely at the code in the feature area. As I have only recently been assigned this feature area, I will need some time to analyze the logic. I will look into this in due time.

[lionel-]

While testing this branch we noticed a problem in the way the afterText matcher is created here:

const afterEnterText = model.getLineContent(lineNumber);

With the rules described in #136593 (comment) and this branch, open a file and paste:

foo %>%
  bar()

Make sure the bar() call is the last line in the file and move the cursor to the end. Type Enter twice, only the first one works.

[ssigwart]

@aiday-mar, thanks for looking at this. Let me know if you want me to make any changes or rebase. This PR is so old. I didn't think it would ever get looked at.

@lionel-, I created this PR about 2.5 years ago, so I'm not 100% sure, but I think that the issue with hitting Enter a second time is a limitation of the way the indentation logic works, but I figured at least it solves the use case of hitting Enter once.

[microsoft-github-policy-service]

@ssigwart the command you issued was incorrect. Please try again.

Examples are:

@ agree

and

@ agree company="your company"

[lionel-]

@ssigwart

The problem is that no new line is entered after the first time. That's because the line I quoted tries to retrieve a line that doesn't exist in the text model and throws an error that is silently swallowed by the type() routine, interrupting the insertion of \n.

[ssigwart]

@lionel-, I was referring to #136593 (comment). I honestly didn't read through your comments about the R language because I thought they were directly towards @aiday-mar and I have never worked with R before.

If this PR does break something that used to work with R, but doesn't with this PR, I definitely should look into solving that. However, at the moment, I don't plan on spending more time on this. I spent a lot of time a few years ago researching issues and submitting PRs, but then I couldn't really get anyone from Microsoft to look at many. So I've given up on spending time on submitting VS Code PRs. I'll pick up work on this again if someone from Microsoft indicates that they plan on reviewing and potentially merging it.