-
Notifications
You must be signed in to change notification settings - Fork 156
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
Conversation
What is that build error about? |
Build and test / Build on windows-latest for repo global.json (pull_request) Failing after 26m |
Now it's |
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.
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 |
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.
We should probably add an IsEmpty
onto the IFSACSourceText
interface.
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.
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 |
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.
Probably should try to not expose indices like this. Maybe use the IsEmpty
suggestion above.
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.
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 |
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.
Hmm this file should be added to the openFiles
and shouldn't be getting here. Probably better to return None
instead
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.
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.
@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. |
Thanks you!! |
You're welcome!! |
* Fix #1906 * You gotta be trolling me * never seen fantomas used this way * More tests * Oops * clarify error * fix grammar
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 filessrc/FsAutoComplete.Core/FileSystem.fs
,src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs
,test/FsAutoComplete.Tests.Lsp/EmptyFileTests.fs
, andtest/FsAutoComplete.Tests.Lsp/Program.fs
.🤖 Generated by Copilot at 8ff5882
🧪🛠️📄
WHY
Fixes ionide/ionide-vscode-fsharp#1906
HOW
🤖 Generated by Copilot at 8ff5882
NamedText
type (link, link, link) and theAdaptiveFSharpLspServer
type (link, link, link) by adding checks for the length of the source text and returning default or empty values as appropriate.