Properly handle console-only fsc options in the VS context

[psfinaki]

fixes #13549

Changes the notion of help options to the console-only related options, which now include --help and --version.
Also added a test (best effort).

Video proof :D

TestCrash.-.Microsoft.Visual.Studio.Int.Preview.2022-08-16.17-46-09.mp4

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[abelbraaksma]

Copying here, as the ‘resolved’ review comments get hidden:

Got it, tx. Does this mean using -version or any illegal option, will now be simply ignored? Or would it allow outputting the version info on each build to the VS console?

[KevinRansom]

@psfinaki --- hey there, there is another case, and it's a little bit wierder.

The command fsc --langversion:4.7 allows you to use ? to get the list of valid versions and then does an exit 1

The handler is here:
https://github.com/dotnet/fsharp/blob/main/src/Compiler/Driver/CompilerOptions.fs#L1078

And the specification is here:
https://github.com/dotnet/fsharp/blob/main/src/Compiler/Driver/CompilerOptions.fs#L1111

I think it may be a lit bit different from your CONSOLE_ONLY items, because it can also contain real options, as well as the help request.

[psfinaki]

@KevinRansom Yep, thanks for the point, I think I will address that separately in a followup. That thing is just ridiculous - the function is called "setLanguageVersion" but actually it sometimes sets the version and sometimes gets the versions. That's like against all the coding principles, so I'll better do a small refactoring and testing around that.

[psfinaki]

@abelbraaksma so normally invalid compiler options are reported. Both in console:

and in VS:

It's just that you hit a special/reserved case with this --version :) This is for getting info about the compiler itself, not giving it any instructions. And so I don't think it makes sense to output this info when building a project - because this info would be unrelated to the project.

So the current implementation will preserve reporting errors in VS for bad options whereas for these special readonly options VS will be just silent. Now as I'm thinking about, I might actually come with a followup to somehow point this out instead of remaining silent - but let this PR just fix the bug itself.

[vzarytovskii]

On the review with @KevinRansom, we've realized that there's an existing mechanism for proper exit from the compiler - using of exiters (with StopProcessing exceptions probably).

For example:

fsharp/src/fsc/fscmain.fs

Lines 77 to 108 in af0015e

	let quitProcessExiter =
	{ new Exiter with
	member _.Exit(n) =
	try
	exit n
	with _ ->
	()
	failwithf "%s" (FSComp.SR.elSysEnvExitDidntExit ())
	}
	// Get the handler for legacy resolution of references via MSBuild.
	let legacyReferenceResolver = LegacyMSBuildReferenceResolver.getResolver ()
	// Perform the main compilation.
	//
	// This is the only place where ReduceMemoryFlag.No is set. This is because fsc.exe is not a long-running process and
	// thus we can use file-locking memory mapped files.
	//
	// This is also one of only two places where CopyFSharpCoreFlag.Yes is set. The other is in LegacyHostedCompilerForTesting.
	CompileFromCommandLineArguments(
	ctok,
	argv,
	legacyReferenceResolver,
	false,
	ReduceMemoryFlag.No,
	CopyFSharpCoreFlag.Yes,
	quitProcessExiter,
	ConsoleLoggerProvider(),
	None,
	None
	)

or:

fsharp/src/Compiler/Service/service.fs

Lines 119 to 129 in af0015e

	let exiter =
	{ new Exiter with
	member x.Exit n = raise StopProcessing
	}
	try
	f exiter
	0
	with e ->
	stopProcessingRecovery e range0
	1

Wondering how easy will it be to plug into the cli options, since it is designed for cases like this and is probably the right way of dealing with it.

[psfinaki]

On the review with @KevinRansom, we've realized that there's an existing mechanism for proper exit from the compiler - using of exiters (with StopProcessing exceptions probably).

@vzarytovskii Good point - I'd say it's a related possible improvement so will do it as a followup.

Tickets for followups: #13748, #13749, #13750

[vzarytovskii]

Ok, I'm going to merge it, and let's have the one with exiter next.

[abelbraaksma]

I finally had some time actually test this locally. Excellent work @psfinaki, solution works like a charm!

[psfinaki]

@abelbraaksma, my pleasure :)