Fix source transformation resolution, JSON deserialization, & transformed source position mapping#1335
Open
hello-42 wants to merge 3 commits intoJohnnyMorganz:pluginsfrom
Open
Fix source transformation resolution, JSON deserialization, & transformed source position mapping#1335hello-42 wants to merge 3 commits intoJohnnyMorganz:pluginsfrom
hello-42 wants to merge 3 commits intoJohnnyMorganz:pluginsfrom
Conversation
Fix undefined behavior from unspecified argument evaluation order When constructing PluginTextDocument, `mapping.getTransformedSource()` and `std::move(mapping)` were passed in the same function call. Since C++ argument evaluation order is unspecified, the move could happen before getTransformedSource() was called, resulting in an empty transformed source string. Extract the transformed source to a local variable before the constructor call to ensure correct sequencing. This fixes the "View Internal Source" debug command returning empty content when plugins return non-empty TextEdits.
Add lua_checkstack to prevent stack overflow in lsp.json.deserialize The pushJsonValue helper function recursively pushes values onto the Lua stack without checking for available space. When parsing large JSON files (like Roblox sourcemaps), this causes a stack overflow assertion failure. Add lua_checkstack(L, 3) at the start of pushJsonValue to ensure sufficient stack space before each recursive call.
Fix virtual method mismatch causing LSP feature failures PluginTextDocument overrides getLineOffsets() to return transformed content line offsets, but the base TextDocument::convertPosition() methods call getLineOffsets() (virtual) while using _content (original). This mismatch caused all position-based LSP features to fail with "invalid string position" errors. Changes: - Add getOriginalLineOffsets() helper with caching for original content - Reimplement convertPosition(lsp::Position) to use original line offsets with proper UTF-16 to UTF-8 conversion - Reimplement convertPosition(Luau::Position) to use original line offsets for the reverse mapping - Add bounds checking in offsetAt() and getText() to prevent out-of-bounds string access This fixes textDocument/completion, textDocument/definition, textDocument/diagnostic, textDocument/inlayHint, and textDocument/signatureHelp when plugins are active.
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
1 participant
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.
I ran into a few issues consecutively when attempting to use the plugins feature:
Any time a non-empty edits table was returned from a plugin's
transformSourcecallback, the process for viewing the internal source would break. This told me that something relevant to transforming the source from the edits was breaking. The fix for this was to extract the transformed source prior topluginDocand then map the transformed source into the document.When using
lsp.json.deserialize(), deserializing mysourcemap.jsonwas causing an unexpected stack overflow for the table returned by deserialization. The fix to the error being emitted internally byluau-lspbelow was to check if there's space on the stack:lapi.cpp(813): ASSERTION FAILED: L->top < L->ci->topTextDocuments andPluginTextDocuments would reference the transformed source'sLocationdata to act as a source for thecompletions,definitions,diagnostics,inlayHints, andsignatureHelpfeatures - which led to the error listed below being emitted internally byluau-lspfor each feature, respectively. The fix was to return the original source'sLocationdata instead.Received response 'textDocument/[...] - (297)' in 3ms. Request failed: invalid string position (-32603)Some notes: