Skip to content

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented Feb 2, 2025

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 baronfel force-pushed the tooltip-nullability branch from cb2f2be to de0b1ec Compare February 2, 2025 23:55
@baronfel baronfel force-pushed the tooltip-nullability branch from de0b1ec to e38caff Compare February 3, 2025 00:07
@baronfel baronfel force-pushed the tooltip-nullability branch from 34e368c to cfe80b7 Compare February 3, 2025 15:39
@baronfel
Copy link
Contributor Author

baronfel commented Feb 3, 2025

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 dotnet/fsharp#17401 for details.

@baronfel
Copy link
Contributor Author

baronfel commented Feb 3, 2025

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

@MangelMaxime
Copy link
Contributor

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.

@TheAngryByrd
Copy link
Member

TheAngryByrd commented Mar 6, 2025

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.

@MangelMaxime
Copy link
Contributor

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?

@TheAngryByrd
Copy link
Member

TheAngryByrd commented Mar 6, 2025

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.

@MangelMaxime
Copy link
Contributor

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).

@TheAngryByrd TheAngryByrd self-assigned this Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hover tooltip does not show nullable values as possibly null
3 participants