-
Notifications
You must be signed in to change notification settings - Fork 833
Fix nullable types formatting in FSharpType.Format and tooltips to include parentheses
#18842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
❗ Release notes required
|
T-Gro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great spot @DedSec256 , you are right.
I might need a little more understanding behind the chosen prec constants.
Btw. the baselines show a drop of parens around some tuples, like:
- "The type '('a * 'b * 'c)'
+ "The type ''a * 'b * 'c'|
@T-Gro Looking at these changes, I still think that we should've used the If I remember correctly the idea to use the |
auduchinok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good, thanks!
ProblemThe main logic for type formatting in fsharp/src/Compiler/Checking/NicePrint.fs Line 956 in 616530c
It is called in several different FCS locations with different default Unfortunately, different parts of the compiler pass different default values — for example, in some diagnostics and tooltips it is called with 5: fsharp/src/Compiler/Checking/NicePrint.fs Lines 1067 to 1069 in 616530c
In other diagnostics, the value 2 is used: fsharp/src/Compiler/Checking/NicePrint.fs Lines 2944 to 2948 in 616530c
Note: The above code with 2 was written in 2014. This leads to the same type, for example, a tuple, being displayed differently in error messages, tooltips, documentation, etc:
In the case of tuples, this is because parentheses around the tuple are placed when fsharp/src/Compiler/Checking/NicePrint.fs Lines 1029 to 1034 in 616530c
As I understand it, the initial idea is to separate nested tuples The rule for parenthesizing null types should consider the following facts:
Therefore, we can choose a value for the condition
I tried using the same value (5), so the tests started failing because of an early inconsistency in tuple representation. What can be done
Pros: consistent Note: considering that the code with 2 was written in 2014, it's possible that the existing logic is a cosmetic error from the beginning or simply doesn't take into account modernity.
Pros: does not affect existing logic, corrects only null types |
|
@brianrourkeboll The image seems broken in your comment. |
It was the image showing the different parenthesization of the same type in these two error messages: > let x : int * int = 1;;
let x : int * int = 1;;
--------------------^
stdin(1,21): error FS0001: This expression was expected to have type
'int * int'
but here has type
'int'
> let y : int * int = null;;
let y : int * int = null;;
--------------------^^^^
stdin(2,21): error FS0043: The type '(int * int)' does not have 'null' as a proper value |
|
I'd expect no parens in both cases 🙂 |
T-Gro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great.
All the baseline changes are an improvement, also in the tuple case now.
Thanks a ton @DedSec256 !
I also understand the prec a little more now - I assume a file comment telling rough meaning of the values in terms of precedence rules could help (or if possible, moving from a numbered static list, e.g. via a plain enum)



Description
Currently, formatting nullable types to strings omits parentheses, resulting in F# type strings that are
Note:
If we take tuple
*as multiplication, and|as a kind of addition, then(string | null) * intwill match(a + 1) * b, but nota + 1 * b. If we assume the prioritization like*>|>→, it would be also consistent with the existing syntax of nullable function types:(int → int) | nullinstead ofint → int | null.Intentions/Quick actions
Besides tooltips and inline hints, this type formatting logic can be used in the IDE for explicit type annotation intentions/quick fixes via
FShapType.Format. Without proper parenthesis handling, user code will be broken:PR
This PR fixes the aforementioned issues for
FSharpType.Formatpublic API and in tooltipsChecklist