extra blanks in all .bsl files

[smoothdeveloper]

Pickling some comments / discussions related to contributor pain with .bsl file formatting:

#6654 (comment)

@dsyme on 13 May 2019 Contributor

Why so many blank lines?

@smoothdeveloper on 14 May 2019 Author Contributor
I mention the issue in this comment: #6654 (comment)

the standard output / error redirect used in the test lead to this kind of output, although when
using FSI in VS or command line, or in VS editing workflow, the messages do not have those extra
CR.

That was the answer:

@KevinRansom commented on 30 Apr 2019
I think it's best to treat those tests as a big blob of "Here there be dragons" … and cautiously migrate out the tests. However, there is a big problem with doing that … the fsharpqa tests can use the F# compiler server, so we don't constantly spin up a new compiler for the tests that need compilation. the Cambridge tests don't do that. So migrating them earns us a slow down.too much to do.

I don't think .bsl are going away, they are now used in the plain fsharp test suite, and they are much more efficient / contributor friendly than say bringing their contents in unit tests in raw strings:

• don't require recompilation if they need any adjustments
• can be looked at / edited with any editor, without looking in large test/code files

Yet, the amount of blank lines makes it hard to get a proper figure of what the compiler actually shows (as indicated by @dsyme comment), and makes it tedious to:

• adjust formatting of error messages (which is an area of compiler that a larger amount of potential contributors can actually contribute)
• perusing the test failures anywhere they show in the CI or unit test runners output

Is the status-quo still the status-quo?

[smoothdeveloper]

One example of why .bsl are better than all inline:

fsharp/tests/fsharp/Compiler/ConstraintSolver/PrimitiveConstraints.fs

Lines 93 to 109 in 0962ff1

	let ``Invalid object constructor``() = // Regression test for FSharp1.0:4189
	CompilerAssert.TypeCheckWithErrorsAndOptions
	[| "--test:ErrorRanges" |]
	"""
	type ImmutableStack<'a> private(items: 'a list) =
	member this.Push item = ImmutableStack(item::items)
	member this.Pop = match items with | [] -> failwith "No elements in stack" | x::xs -> x,ImmutableStack(xs)
	// Notice type annotation is commented out, which results in an error
	new(col (*: seq<'a>*)) = ImmutableStack(List.ofSeq col)
	"""
	[| FSharpErrorSeverity.Error, 41, (4, 29, 4, 56), "A unique overload for method 'ImmutableStack`1' could not be determined based on type information prior to this program point. A type annotation may be needed. Candidates: new : col:'b -> ImmutableStack<'a>, private new : items:'a list -> ImmutableStack<'a>"
	FSharpErrorSeverity.Error, 41, (5, 93, 5, 111), "A unique overload for method 'ImmutableStack`1' could not be determined based on type information prior to this program point. A type annotation may be needed. Candidates: new : col:'b -> ImmutableStack<'a>, private new : items:'a list -> ImmutableStack<'a>"
	FSharpErrorSeverity.Error, 41, (8, 30, 8, 60), "A unique overload for method 'ImmutableStack`1' could not be determined based on type information prior to this program point. A type annotation may be needed. Candidates: new : col:'b -> ImmutableStack<'a> when 'b :> seq<'c>, private new : items:'a list -> ImmutableStack<'a>"
	FSharpErrorSeverity.Error, 696, (8, 30, 8, 60), "This is not a valid object construction expression. Explicit object constructors must either call an alternate constructor or initialize all fields of the object and specify a call to a super class constructor." |]

If we are going to have very large amount of tests around compiler error messages (which aren't all covered AFAIU), the approach above is less friendly / less efficient:

• can't look / edit at the F# code as if it was in a plain .fs or .fsx file
• can't look at the expected output next to the actual output like in plain .bsl .err files
• can't change the expected message without recompiling the tests (this takes more time than adjusting a .bsl and running the same already compiled test)
• can't fix a bunch of expected output in a single go like it is possible to do with .bsl files
• harder to review / compare / merge diffs (code conflicts in .fs file with many tests are harder to manage than in .bsl or single input source file)

[smoothdeveloper]

I'd like to add that investing in the proper .bsl and individual .fs or .fsx files infrastructure will also be valuable to streamline the cost of maintaining the awesome work @baronfel has undertaken in dotnet/docs#16380 down the road.

Right now, lots of error messages are probably not covered under tests, each documentation page that is added embeds code samples and compiler output, which are 1-to-1 with what the source files / baseline files pairs are.

A system could be engineered so that updating those samples and output is automatical on new compiler release, based on the contents of this repository.

[cartermp]

The status quo is probably going to remain for quite some time, but we're keen on moving away from how fsharpqa works today and making inputs/output inline in F# code. The benefit of having everything in one place and using a framework that tooling actually understands (such as CI reports!) far outweighs the current way things are done for us.

[smoothdeveloper]

I'm closing the issue as #8219 is merged (thanks @KevinRansom / @cartermp!).

We can discuss separately and later about the development & contribution workflow to improve compiler message more efficiently:

• making it easy to improve a message without having to manually edit lots string literals and recompiles
• how to keep the work from @baronfel in sync without involving any human efforts for the code samples and their output
• having them all covered under tests
• keep and improve all the nice things such as CI reports (thanks @brettfo) while enabling fast feedback loop (with no recompile needed in that loop) for people working with the codebase

Compiler messages are an important gateway for new adopters and new contributors.

[smoothdeveloper]

@cartermp just to address your comment succintly, quoting you: #8094 (review)

Since this is an error message update there's nothing special here, just copy/pasting the new expected error message into the baseline. It's kind of annoying.

With baseline files, this can be assisted and not annoying at all, by just running a script (bonus: no recompile of test project to run test again until all is passing green), but by keeping all in literals that are to be updated manually only in the IDE in many places, that indeed makes it "kind of annoying" => not inviting more contributions.

You can repeat that cycle for each minute adjustment of the error message in the context of a contribution, "kind of annoying" at each adjustment, or assisted with very little user interaction beside checking the diff which you can't avoid.

Hope that makes sense.