Merged
Conversation
Allows to easily debug many cases by just looking at the log.
Using "URI.Path + URI.Document" to convert TURI to a filename is not good on Windows, as it results in invalid filenames like "/d:/cygwin64/home/michalis/installed/fpclazarus/3.2.2-lazarus2.2/lazarus/components/codetools/unitdictionary.pas". The URIToFilename from URIParser unit instead provides a cross-platform correct solution. Also, we improve error reporting in case CodeToolBoss.LoadFile fails: clear error message instead of Access Violation at Code.Source usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The LSP server was always crashing on Windows with Access Violation.
The core culprit was: one cannot use logic like
on Windows. It will result in an additional forward slash being added at the beginning of the filename, and the resulting path is not a valid filename on Windows, that can be opened. For example the filename was
/d:/cygwin64/home/michalis/installed/fpclazarus/3.2.2-lazarus2.2/lazarus/components/codetools/unitdictionary.pas.utextdocument unit used this faulty logic a few times. In 3x cases there was a check whether the file was opened successfully with a clear exception, but the
TextDocument_DidOpenandTextDocument_DidChangedidwhich crashes with Access Violation when filename was invalid, because
CodeToolBoss.LoadFilethen returnsnil.I fixed the core issue, and made some related things more reliable and easier to debug (this should help also on non-Windows):
To convert URI to a filename,
URIParserstandard unit contains a readyURIToFilenameroutine. This deals with all cross-platform stuff for us. I changed the code to use it everywhere.( Castle Game Engine is using
URIToFilenametoo, for all our URI->filename logic, through https://github.com/castle-engine/castle-engine/blob/master/src/files/castleuriutils.pas#L910 . It is reliable. )Just in case, I added check for
Code = nilafterCode := CodeToolBoss.LoadFile. It should not fail anymore, but if it will -- it will show a nice exception likeFATAL EXCEPTION: Unable to load file ....instead of cryptic Access Violation.In castle-engine@78c7163 I extended the exception message with a backtrace. This uses
DumpExceptionBackTracefrom FPC, wrapped in easy functionDumpExceptionBackTraceToString(from Castle Game Engine https://github.com/castle-engine/castle-engine/blob/master/src/base/castleclassutils.pas#L2257 , here simplified a little).In the end you will get better exception messages with backtrace in the log, like this:
In castle-engine@4767a32 I also did a few fixes to have the
DebugLogoutput use "native" line endings, i.e.#13#10on Windows,#10on Unix. Previous code was using#10in most places, which is non-standard for Windows, and also was getting mixed with#13#10produced byDumpExceptionBackTraceon Windows.So to have consistent line endings, standard on Windows, I use
LineEndingeverywhere.Finally in castle-engine@f6410d1 I changed the code to use
FileNameToURI, instead of doing it by'file://' + CurPos.Code.Filename.On Windows, the previous "manual" method of concatenation generates invalid URL, as it misses an additional slash. See https://en.wikipedia.org/wiki/File_URI_scheme#Windows_2 how the file URL on Windows should look like.
The practical effect of this problem was that jumping to identifiers, e.g. by Ctrl + click on a method name in VS Code, wasn't working (VS Code reported error as on the screenshot). After the fix, it works perfectly.
Note: Originally I did these changes on https://github.com/castle-engine/pascal-language-server branch that adds also CGE-specific modifications and support for log file defined in
c:/Users/<username>/AppData/Local/pasls/castle-pasls.inifile. This made the debugging even easier, I saw theFATAL EXCEPTION...error in a regular text file. But I don't submit it with this PR, I'm myself not sure about the elegance of this INI file support (it seems cleaner to get all options through LSP initialization options, not some additional INI file).Tested with Emacs on Windows and VS Code on Windows. After the fixes, they work as nicely as on Linux :)