-
-
Notifications
You must be signed in to change notification settings - Fork 202
Adaptation to breaking FCS changes in sdk 9.0 and 10.0 #3167
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
{ | ||
"sdk": { | ||
"version": "8.0.400", | ||
"version": "9.0.300", | ||
"rollForward": "latestPatch" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,7 @@ let ``#help without string`` () = | |
#help List.map | ||
""" | ||
|
||
// As of F# 10.0, warn directives are treated as trivia like #if, so argruments are not formatted | ||
[<Test>] | ||
let ``#nowarn with integer`` () = | ||
formatSourceString | ||
|
@@ -259,5 +260,5 @@ let ``#nowarn with integer`` () = | |
|> should | ||
equal | ||
""" | ||
#nowarn 1182 | ||
#nowarn 1182 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, seems like a step back, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warn directives are now handled exactly like conditional directives as whole-line trivia. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, yes, but you don't have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the numbers actually at some point (as int only, as they are no longer parsed by the parser). But when I saw how the conditional directives are treated, I went for the same simple whole-line-trivia approach. After all, both conditional and warn directives are preprocessor directives that are not really part of the proper syntax tree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't say I really follow here, looking at this example, the AST does have proper nodes for the expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I believe the expressions are only used to direct the computation of the different trees. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, I assumed wrongly that we were using the AST node to actually print the hash directive expressions, looks like we indeed took a shortcut and took the original source. We would look at the nodes honestly. Anyway, circling back to the new thing, how are you getting the values of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Long story :-). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the WarnDirectives entry in the AST only for Fantomas. (Again, like the ConditionalDirectives entry.) |
||
""" |
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.
I no longer remember how this stuff works, but can we stick to dotnet 8 SDK and versions? Or do you really need 9?
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.
Due to the new nullness-related constructs, compiling the current compiler service requires 9.0 or higher for both fsc and FSharp.Core.
That's why I wrote above that this PR might be a draft one for quite some time.
I am assuming you might want to wait for the LTS SDK 10.
Regarding features I don't think there is a big pressure to move. The current Fantomas can deal with sources that contain nullness constructs or #warnon.
It is just that the changes accumulate. I naively just wanted to make the changes for "scoped nowarn" but found I had to deal with three other breaking changes also.
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.
Do you have any more details on that? What exactly in FSharp.Core 9 does any of this depend on?
Uh oh!
There was an error while loading. Please reload this page.
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.
It is all about nullness.
If I compile with SDK 9.0.300 and FSharp.Core 8.0.100, I get 85 errors in the Fantomas.FCS project, mostly about unknown
objnull
andNonNull
.