FsAutoComplete icon indicating copy to clipboard operation
FsAutoComplete copied to clipboard

Support rendering `| null` syntax for nullable values in tooltips

Open baronfel opened this issue 11 months ago • 7 comments

Fixes #1352 by checking the nullability information on FSharpType and using that to determine if we should slap a | null on the formatted type.

baronfel avatar Feb 02 '25 23:02 baronfel

This got really hairy. The code seems to work, but for scripts specifically it requires passing --checknulls+ to the checker manually - there doesn't seem to be an in-script way to light up nullability like #nullable enable. See https://github.com/dotnet/fsharp/issues/17401 for details.

baronfel avatar Feb 03 '25 15:02 baronfel

We may enable nulls for all .NET 9 usage of scripts as a result.

baronfel avatar Feb 03 '25 15:02 baronfel

If I understand correctly, for F# project FSAC will respect the <Nullable> configuration from the project?

But for scripts we don't have an equivalent out of the box from FCS?

If yes, instead of always passing --checknulls+ for all .NET 9 scripts usage, we could perhaps have a setting in the LSP/Ionide layer allowing the user to toggle the flag on/off.

Even, if we require a restart of the LSP/Ionide for it to be considered I think this is fine.

MangelMaxime avatar Mar 06 '25 13:03 MangelMaxime

We already do have FSharp.fsiExtraParameters which I think all you have to do is close and reopen the .fsx file.

Though it would be "cute" if there was some metadata we could store at the top of the file that passed in the config flags too. Kind of like a shebang.

TheAngryByrd avatar Mar 06 '25 14:03 TheAngryByrd

We already do have FSharp.fsiExtraParameters which I think all you have to do is close and reopen the .fsx file.

True, not sure how many people would be comfortable with this setting versus a checkbox.

But at the same time, I am not sure how many people are making changes to their Ionide settings either.

Though it would be "cute" if there was some metadata we could store at the top of the file that passed in the config flags too. Kind of like a shebang.

Indeed, but this the job of FCS no? Or you want for FSAC do detect a certain metadata by reading the script file?

MangelMaxime avatar Mar 06 '25 14:03 MangelMaxime

True, not sure how many people would be comfortable with this setting versus a checkbox. But at the same time, I am not sure how many people are making changes to their Ionide settings either.

A new setting has to exist for a long time and can get confusing when this setting is a union of it and another setting. Like what happens when you set the checkbox to true but explicitly write --checknull- in fsiParameters? I'd prefer having documentation pointing to how to configure your fsi correctly as you'd need it anyway if you want to run the script via CLI.

Indeed, but this the job of FCS no? Or you want for FSAC do detect a certain metadata by reading the script file?

Probably yes, but I think they'd get to a chicken/egg problem where the script is already started via fsi so it might be hard to change settings. (I'm not too clear with their pipeline though to know).

If someone did have a shebang of like #!/usr/bin/env -S dotnet fsi --checknulls+, we could parse it and prefer it as the settings. It would get weird with scripts referencing other scripts though. Probably just prefer the script you opened. Also I'm unsure if having multiple fsi sessions in the same process would be a problem.

TheAngryByrd avatar Mar 06 '25 14:03 TheAngryByrd

True, not sure how many people would be comfortable with this setting versus a checkbox. But at the same time, I am not sure how many people are making changes to their Ionide settings either.

A new setting has to exist for a long time and can get confusing when this setting is a union of it and another setting. Like what happens when you set the checkbox to true but explicitly write --checknull- in fsiParameters? I'd prefer having documentation pointing to how to configure your fsi correctly as you'd need it anyway if you want to run the script via CLI.

I would expect the checkbox to have priority and hopefully passing it last would get precedence.

But, I agree this could be confusing.

Having documentation is fine to me, and we could also have a command in Ionide to add/remove the setting from fsiParameters directly. Even, if it could be tricky to have the right UX because of the 3 levels of configuration that VSCode supports (global, user, workspace).

MangelMaxime avatar Mar 06 '25 14:03 MangelMaxime