Skip to content
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

Latest fcs #1122

Merged
merged 13 commits into from
Jul 1, 2023
Merged

Latest fcs #1122

merged 13 commits into from
Jul 1, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jun 20, 2023

WHAT

🤖 Generated by Copilot at b09798e

Updated the code base to use the latest F# compiler service API, which has some breaking changes in the syntax tree representation and the tokenizer. Fixed the code fixes and the untyped AST utilities to work with the new API.

🤖 Generated by Copilot at b09798e

FSharp.Compiler.Service
Updated to latest version
Code fixes follow

🔄🐛🚀

WHY

HOW

🤖 Generated by Copilot at b09798e

  • Update the source URL and the version constraint for the FSharp.Compiler.Service package in paket.dependencies to use the latest dotnet7 feed and prerelease (link, link)
  • Update the pattern matching for the SynPat.Tuple cases in InlayHints.fs and UnionPatternMatchCaseGenerator.fs to use the named field elementPats instead of the deprecated tuple syntax (link, link)
  • Update the call to the FSharpSourceTokenizer constructor in Lexer.fs to pass an optional argument for the target framework, which is required by the latest version of the F# compiler service (link)
  • Update the pattern matching for the SynPat.Tuple and SynSimplePats.SimplePats cases in UntypedAstUtils.fs to use the named fields instead of the deprecated tuple syntax, and remove the SynSimplePats.Typed case, which is no longer used by the F# compiler service (link, link, link)
  • Remove the wildcard case for the pattern matching in the VisitModuleDecl method in AddExplicitTypeAnnotation.fs, as it is no longer needed by the logic of the code fix (link)

Contains API changes for the latest FCS (43.7.400-preview.23319.6)

@baronfel
Copy link
Contributor

Looks like StructuredLogging versioning mismatches - can you bump that dependency and rerun?

@@ -48,7 +48,7 @@ module Lexer =
|> Seq.choose (fun s -> if s.StartsWith "--define:" then Some s.[9..] else None)
|> Seq.toList

let sourceTokenizer = FSharpSourceTokenizer(defines, Some "/tmp.fsx")
let sourceTokenizer = FSharpSourceTokenizer(defines, Some "/tmp.fsx", None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should check if we have langVersion available via the project options - we should try to provide it here if possible. This can be a follow-up issue if you don't want to tackle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it.

@nojaf
Copy link
Contributor Author

nojaf commented Jun 21, 2023

Some tests are failing because properties now have a symbol for both getters and setters.

For example:
https://github.com/fsharp/FsAutoComplete/blob/43affdec70a82b69d52aaadeed524b05192998f1/test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs#L904-L915

The code on the main branch currently finds a single symbol (the getter), found usages of that and thus does not return the code action.

In this PR, the setter symbol was returned and does not contain any usage.

See dotnet/fsharp#15213 why properties now have two symbols.
Note, that is correct behaviour.

In order to get both symbols, dotnet/fsharp#15285 was added and I think some of the code will need to be extended to take this into account.

//cc @dawedawe

@@ -115,7 +115,7 @@ let fix (getParseResultsForFile: GetParseResultsForFile) : CodeFix =
let commentsAndPos = tryGetCommentsAndSymbolPos parseAndCheck.GetAST fcsPos

match commentsAndPos with
| Some(docLines, docRange, symbolPos) ->
| Some(docLines, docRange, symbolPos, isAutoProperty) ->
let lineStrOfSymbol = _sourceText.GetLine symbolPos |> Option.defaultValue ""
let signatureData = parseAndCheck.TryGetSignatureData symbolPos lineStrOfSymbol
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signatureData for the setter was returned here.
This does indeed have a member parameter.

dawedawe and others added 2 commits June 24, 2023 10:00
…Or is it?

Meanwhile, adjust the reflection code to the changes in FCS.
Adjust reflection code to changes i FCS
@baronfel
Copy link
Contributor

Something seems to be pretty wrong in find all references now for some reason :(

@dawedawe
Copy link
Contributor

dawedawe commented Jun 26, 2023

Something seems to be pretty wrong in find all references now for some reason :(

I think that's caused by failing reflection code. I tried to fix that in ionide/proj-info#192
At least, I was able to get a green test pipeline with that locally.

dawedawe and others added 2 commits June 27, 2023 10:33
- Update to latest FCS beta package
Update to Ionide.ProjInfo nightly and latest FCS beta
@nojaf
Copy link
Contributor Author

nojaf commented Jun 28, 2023

@baronfel could you re-run that failing Mac build? This might be a hiccup...

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks so much for this! Left some feedback.

symbolUse
}

let symbolUseWorkspace2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep a separate implementation or could you leave a comment for why there's a second implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, we still mean to clean that up. Not sure if we should rename or consider moving everything to the v2 implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a version that uses v2 for everything and takes the simplest approach 😏
nojaf#5
@TheAngryByrd Let me know, if you prefer a version that keeps the symbol(s) in it's return type.

@baronfel
Copy link
Contributor

baronfel commented Jul 1, 2023

Oh hey - we're green! Thanks folks!

@baronfel baronfel merged commit 7fc82db into ionide:nightly Jul 1, 2023
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.

4 participants