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

Fix ionide/ionide-vscode-fsharp#1906 #1152

Merged
merged 7 commits into from
Sep 1, 2023
Merged

Conversation

Happypig375
Copy link
Contributor

@Happypig375 Happypig375 commented Aug 12, 2023

WHAT

🤖 Generated by Copilot at 8ff5882

This pull request improves the support for empty or unsaved files in the F# language server and the NamedText type. It fixes potential exceptions, skips unnecessary operations, and adds tests for the language server features on empty files. It affects the files src/FsAutoComplete.Core/FileSystem.fs, src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs, test/FsAutoComplete.Tests.Lsp/EmptyFileTests.fs, and test/FsAutoComplete.Tests.Lsp/Program.fs.

🤖 Generated by Copilot at 8ff5882

The F# language server is great
But it had some issues of late
With empty files
It threw some trials
So this pull request fixes its state

🧪🛠️📄

WHY

Fixes ionide/ionide-vscode-fsharp#1906

HOW

🤖 Generated by Copilot at 8ff5882

  • Prevent errors and handle empty files in the NamedText type (link, link, link) and the AdaptiveFSharpLspServer type (link, link, link) by adding checks for the length of the source text and returning default or empty values as appropriate.

@Happypig375 Happypig375 changed the title Fix #1906 Fix ionide/ionide-vscode-fsharp#1906 Aug 12, 2023
@Happypig375
Copy link
Contributor Author

What is that build error about?

@Happypig375
Copy link
Contributor Author

Build and test / Build on windows-latest for repo global.json (pull_request) Failing after 26m
Build and test / Build on macos-latest for repo global.json (pull_request) Failing after 29m
Build and test / Build on macos-latest for 7.0 preview (pull_request) Failing after 3m

@Happypig375
Copy link
Contributor Author

Now it's
Build and test / Build on macos-latest for repo global.json (pull_request) Failing after 27m
Build and test / Build on macos-latest for 6.0 stable (pull_request) Failing after 30m
Build and test / Build on macos-latest for 7.0 preview (pull_request) Failing after 2m

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.

Thanks for this! Left some comments.

Also did this only happen with NamedText and not happen with the RoslynSourceText or do we need to have changes applied to that as well?

if not (Range.rangeContainsRange x.TotalRange m) then
// indexing into first line of empty file can be encountered when typing from an empty file
// if we don't check it, GetLineString will throw IndexOutOfRangeException
if (x :> ISourceText).GetLineCount() = 0 then
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add an IsEmpty onto the IFSACSourceText interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the use of this IsEmpty widespread enough to make a change to public API?

let config = config |> AVal.force
do! builtInCompilerAnalyzers config volatileFile parseAndCheck
do! runAnalyzers config parseAndCheck volatileFile
if volatileFile.Source.Length = 0 then
Copy link
Member

Choose a reason for hiding this comment

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

Probably should try to not expose indices like this. Maybe use the IsEmpty suggestion above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by indices? There is no index here. It's just a length test.


else // When a user does "File -> New Text File -> Select a language -> F#" without saving, the file won't exist
return
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this file should be added to the openFiles and shouldn't be getting here. Probably better to return None instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will still be warnings written to VSCode output if I return None here. Returning this value, meanwhile, doesn't result in warnings written to VSCode output.

@Happypig375
Copy link
Contributor Author

Happypig375 commented Aug 13, 2023

@TheAngryByrd No idea about that. I only verified with a new VSCode instance and typing from there. However, since tests test both NamedText and RoslynSourceText, and I only had to modify NamedText to make tests pass, I think RoslynSourceText works fine.

@TheAngryByrd TheAngryByrd self-assigned this Aug 17, 2023
@TheAngryByrd TheAngryByrd self-requested a review August 29, 2023 19:22
@TheAngryByrd
Copy link
Member

Thanks you!!

@TheAngryByrd TheAngryByrd merged commit daf3619 into ionide:main Sep 1, 2023
9 checks passed
@Happypig375
Copy link
Contributor Author

You're welcome!!

nojaf pushed a commit to nojaf/FsAutoComplete that referenced this pull request Nov 3, 2023
* Fix #1906

* You gotta be trolling me

* never seen fantomas used this way

* More tests

* Oops

* clarify error

* fix grammar
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.

New Text File -> Select a language -> F#, Ionide writes error to output
2 participants