Adaptation to breaking FCS changes in sdk 9.0 and 10.0 #3167
Conversation
| @@ -4,7 +4,7 @@ | |||
| <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <PackageVersion Include="FSharp.Core" Version="8.0.100"/> | |||
| <PackageVersion Include="FSharp.Core" Version="9.0.300"/> | |||
There was a problem hiding this comment.
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.
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.
Do you have any more details on that? What exactly in FSharp.Core 9 does any of this depend on?
There was a problem hiding this comment.
Do you have any more details on that? What exactly in FSharp.Core 9 does any of this depend on?
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 and NonNull.
| @@ -259,5 +260,5 @@ let ``#nowarn with integer`` () = | |||
| |> should | |||
| equal | |||
| """ | |||
| #nowarn 1182 | |||
| #nowarn 1182 | |||
There was a problem hiding this comment.
Hmm, seems like a step back, though.
What AST does this give?
There was a problem hiding this comment.
Warn directives are now handled exactly like conditional directives as whole-line trivia.
https://github.com/dotnet/fsharp/blob/main/tests/service/data/SyntaxTree/WarnScope/WarnScope.fs.bsl
There was a problem hiding this comment.
Well, yes, but you don't have the 1182 in the AST?
There was a problem hiding this comment.
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.
I can't say I really follow here, looking at this example, the AST does have proper nodes for the expression.
Your nowarn and warn only have the range?
There was a problem hiding this comment.
Yes, but I believe the expressions are only used to direct the computation of the different trees.
If I understand it correctly (with my limited knowledge of Fantomas), they are not used in formatting.
(EDIT: if the question is based on my mentioning "proper syntax tree" - that was just a remark from a theoretical point of view.)
There was a problem hiding this comment.
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 nowarns later in the filtering?
There was a problem hiding this comment.
Long story :-).
For the "warn scopes" functionality to make sense, #nowarn and #warnon must be allowed on any line (also like in the middle of an expression).
Therefore, we can no longer parse them in the parser. We parse them rather during lexing and store them first temporarily in a lexbuf BufferLocalStore (similar to #ifdef and xml comments) and later in DiagnosticOptions, which is available where the diagnostic filtering takes place. So, all outside of compilation proper (parsing, syntax trees (except for the trivia), type checking, code gen etc).
There was a problem hiding this comment.
I added the WarnDirectives entry in the AST only for Fantomas. (Again, like the ConditionalDirectives entry.)
src/Fantomas.Core/ASTTransformer.fs
Outdated
| @@ -966,22 +966,22 @@ let mkExpr (creationAide: CreationAide) (e: SynExpr) : Expr = | |||
| | SynExpr.AddressOf(false, e, _, StartRange 2 (ampersandToken, _range)) -> | |||
| ExprSingleNode(stn "&&" ampersandToken, false, false, mkExpr creationAide e, exprRange) | |||
| |> Expr.Single | |||
| | SynExpr.YieldOrReturn((true, _), e, StartRange 5 (yieldKeyword, _range)) -> | |||
| | SynExpr.YieldOrReturn((true, _), e, StartRange 5 (yieldKeyword, _range), _) -> | |||
There was a problem hiding this comment.
We should not ignore the trivia here, replace yieldKeyword with this instead: https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntaxtrivia-synexpryieldorreturntrivia.html
There was a problem hiding this comment.
Mmh. I guess I know too little about ASTs and Oaks. But isn't yieldKeyword just the range of the "yield"? It looks right to me as is. While SynExprYieldOrReturnTrivia is the type of the trivia-range-wrapper that I ignored. But what should I do with this range?
There was a problem hiding this comment.
What we do with StartRange 5 (yieldKeyword, _range)) is creating a new range for the yield keyword using the range of the entire SynExpr.
But since the trivia, now hold the same information, we can ignore the range and use the trivia to get the yieldKeyword range instead.
src/Fantomas.Core/ASTTransformer.fs
Outdated
| @@ -2809,7 +2807,7 @@ let mkMemberDefn (creationAide: CreationAide) (md: SynMemberDefn) = | |||
| let memberDefinitionRange = md.Range | |||
|
|
|||
| match md with | |||
| | SynMemberDefn.ImplicitInherit(t, e, _, StartRange 7 (mInherit, _)) -> | |||
| | SynMemberDefn.ImplicitInherit(t, e, _, StartRange 7 (mInherit, _), _) -> | |||
There was a problem hiding this comment.
Same remark as above, grab mInherit from trivia: https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntaxtrivia-synmemberdefninherittrivia.html
This is a draft PR to accommodate breaking changes of FCS (FSharp.Compiler.Service) in .net SDK 9.0 and 10.0, namely
The
global.jsonandFSharp.Coreversions had to be pushed from 8.0 to 9.0 to enable compilation of the current FCS version.I expect this PR to stay for quite some time in draft, but I wanted to raise it as long as my memory of the scoped nowarn changes was still fresh.
I am willing to keep it updated and to extend it if further breaking FCS changes come up.