Parens: tweak sensitive indentation handling

[brianrourkeboll]

Another followup to #16079.

• Dedent by one space in certain scenarios.

There are certain cases where we must consider both the context outside of a pair of parentheses as well as the inside in order to reintegrate the inner construct into the outer context after removing the parentheses. That is, sometimes we cannot simply remove the parentheses in place, but we must instead shift the entire parenthesized multiline expression left by one space.

For example:

let _ =
    1,
    (true
     || false
     || true),
    1

must become

let _ =
    1,
    true
    || false
    || true,
    1

not

let _ =
    1,
     true
     || false
     || true,
    1

Or here's a real-world example from FSharp.Core:

fsharp/src/Compiler/Utilities/sformat.fs

Lines 891 to 901 in 9e7ec64

	ty.InvokeMember(
	name,
	(BindingFlags.GetProperty
	||| BindingFlags.Instance
	||| BindingFlags.Public
	||| BindingFlags.NonPublic),
	null,
	obj,
	[||],
	CultureInfo.InvariantCulture
	)

[psfinaki]

Cool, thanks!

[brianrourkeboll]

Hmm, I think I see one bug... Let me fix that.

[brianrourkeboll]

Tricky, tricky:

fsharp/src/FSharp.Core/eventmodule.fs

Lines 74 to 79 in 9a0b9bf

	sourceEvent.Add(fun args2 ->
	(match lastArgs with
	| None -> ()
	| Some args1 -> ev.Trigger(args1, args2))
	lastArgs <- Some args2)

[brianrourkeboll]

Expect some more PRs after this one, unfortunately—running fix-all on FSharp.Core is surfacing some other oddities I hadn't thought of, like the fact that parens aren't needed after new in this:

type T (x, y) =
    new (x) = T (x, 3) // Can remove parens: `new x = T (x, 3)`.

or in this:

type T (x, y) =
    new (x, y, z) = T (x, y)
    new (x) = T (x, 3) // Can remove parens: `new x = T (x, 3)`.

but they are required if there's another constructor and it comes after:

type T (x, y) =
    new (x) = T (x, 3) // Cannot remove; removing the parens causes parse errors below.
    new (x, y, _z) = T (x, y)

Or the fact that this is fine:

type C = abstract M : unit -> unit
let _ = { new C with override _.M () = () } // I.e., `override _.M (())` can be simplified to `override _.M ()`

but this requires (()) for the override:

type C<'T> = abstract M : 'T -> unit
let _ = { new C<unit> with override _.M (()) = () }

Real example:

fsharp/src/FSharp.Core/printf.fs

Lines 1074 to 1078 in 9a0b9bf

	let StringBuilderPrintfEnv<'Result>(k, buf) =
	{ new PrintfEnv<Text.StringBuilder, unit, 'Result>(buf) with
	override _.Finish() : 'Result = k ()
	override _.Write(s: string) = ignore(buf.Append s)
	override _.WriteT(()) = () }

[psfinaki]

We are totally ready for the followups :)
Yeah looks like you are facing more and more inconsistencies in the compiler... Feel free to create issues about those, we might do something with the parser to eliminate some root causes.

[psfinaki]

@brianrourkeboll this LGTM - is there anything else you plan to add here?

[brianrourkeboll]

@brianrourkeboll this LGTM - is there anything else you plan to add here?

@psfinaki Nope, this one should be all set.

[majocha]

It seems this test case started to fail at some point:

fsharp/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs

Lines 784 to 790 in 9e357ca

	"
	(match x with
	| 1 -> ()
	| _ -> ())
	y
	",

Apparently because it's not being caught by the diagnostics.