Error for primary constructor in delegate definition

[edgarfgp]

Attempt to fix #8299

[vzarytovskii]

We try not to add any new tests to the fsharpqa if not necessary, I've mentioned about the ComponentTests in the #8299 (comment), sorry for the delay.

[edgarfgp]

No worries :) . Thanks for taking the time . I will update this PR according with your suggestion

[edgarfgp]

@vzarytovskii I realised that the #8299 talks about to show a syntax error for this rather that a compilation error.

I’m a little confused the test that I added in this PR they succeeded I was expecting them to fail As if you send interactive it show an error 😢.

[vzarytovskii]

Yeah, when you call compile it will use FCS (i.e. compiler), and not fsi/interactive session.
Did you intend to test compiler or fsi?

[edgarfgp]

Yeah, when you call compile it will use FCS (i.e. compiler), and not fsi/interactive session.
Did you intend to test compiler or fsi?

I think the expectation would to have one for compiler(that show a syntax error) and one for fsi/interactive session(that shows an error) . So they are both consistent .

[vzarytovskii]

Yeah, so for compiler, you can simply leave this this test, and then fix the compiler itself, so the test passes (i.e. the compiler expected to fail).
For the FSI, you can use the runFsi function, here's the example of such test:

fsharp/tests/FSharp.Compiler.ComponentTests/Scripting/Interactive.fs

Lines 18 to 29 in d307f8d

	module ``External FSI tests`` =
	[<Fact>]
	let ``Eval object value``() =
	Fsx "1+1"
	|> runFsi
	|> shouldSucceed
	[<Fact>]
	let ``Invalid expression should fail``() =
	Fsx "1+a"
	|> runFsi
	|> shouldFail

Feel free to ping me on discord/slack if you have any questions.

[edgarfgp]

@vzarytovskii While working on this PR and asking in discord I wonder if there is Is anything preventing us to raise this during parsing and not in CheckDeclarations ?

[auduchinok]

@edgarfgp The parts that parse the constructor and the type body are implemented in different grammar rules and doing it in some parent rule would mean checking declarations produced by inner rules. The parser doesn't seem to be the right place to do this, as it's the job of the checker (that does a lot of similar checks already). Since this error is already reported for other kinds of functional F# types, the best place to implement reporting it for delegates would be somewhere near the existing cases.

[edgarfgp]

I have made the changes and wrote unit test and my test all passing locally but the one the CI gates is failing due to neg06.bsl differed: a bug or baseline may need updating

[vzarytovskii]

I have made the changes and wrote unit test and my test all passing locally but the one the CI gates is failing due to neg06.bsl differed: a bug or baseline may need updating

Not yet sure why it's failing, gonna look at it later today or tomorrow morning.

Edit: oh, it seems the baseline in question needs updating, it doesn't expect new error to happen there, but it is.

[edgarfgp]

@vzarytovskii CI got even more angry with my attempt of updating the baseline tests 😅. I think I’m going to take couple days to try to understand this and then come back . I think I’m close to have this green

[vzarytovskii]

Oh, yeah, there was some infra issues.

[vzarytovskii]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[auduchinok]

I left some comments, @edgarfgp please take a look 🙂

[edgarfgp]

@vzarytovskii Something has changed on main in the code formatting and the CI fails now . Should I run fantomas on those files and commit the changes ?

[vzarytovskii]

@vzarytovskii Something has changed on main in the code formatting and the CI fails now . Should I run fantomas on those files and commit the changes ?

We'll fix it in main.

[nojaf]

@edgarfgp congratulations on this PR!
A fine addition!

[edgarfgp]

@edgarfgp congratulations on this PR! A fine addition!

Thanks :) . I got inspired by your work on Fantomas

[vzarytovskii]

Thanks a lot for this @edgarfgp!

@KevinRansom @dsyme this LGTM, shall we get it in?

[dsyme]

Looks good to me