Desugar conditional sends#834
Closed
amomchilov wants to merge 43 commits intoAlex/extract-desugarMethodCallfrom
Closed
Desugar conditional sends#834amomchilov wants to merge 43 commits intoAlex/extract-desugarMethodCallfrom
amomchilov wants to merge 43 commits intoAlex/extract-desugarMethodCallfrom
Conversation
This was referenced Jan 9, 2026
Closed
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Jan 9, 2026
amomchilov
force-pushed
the
Alex/extract-desugarMethodCall
branch
from
092387a to
d9b0e05
Compare
amomchilov
force-pushed
the
Alex/desugar-csend
branch
from
91e64a9 to
74e3928
Compare
* Include parens in multi-target locs * Delete now-unused loc helpers * Fix location of `to_ary` call
* extract code to check for visible_to * inline visibilityApplies * delete this ENFORCE * no-op: reorganize * no-op: causesModularityError -> validToImport * check visible_to at call sites and don't add imports that would violate visible_to * test for gen-packages mode * test for call sites * use absl::c_any_of
* Skip loop if there's no splats at all * Move `messageLoc` declaration * Narrow scope of `blockPassLoc` * Move `blockExpr` declaration up * Move symbol proc desugaring up * Simplify checks for `blockPassArg` * Simplify checks for `blockExpr` * Inline `blockPassArgIsSymbol` * Invert `if` * Narrow scope of block-related variables * Narrow scope of `blockStatsStore` check * Inline `methodName` * Rename `name` → `methodName` * Narrow scope of `prismBlock` * FIXME: move this comment * Extract `desugarLiteralBlock()` Co-authored-by: Jesse Johnson <jesse.johnson@shopify.com> * Extract `desugarBlockPassArgument()` Co-authored-by: Jesse Johnson <jesse.johnson@shopify.com> * Compare NameRefs instead of strings * Move receiver handling up * Lift out `argumentsNode` local * Reuse `receiverNode` local * Rename `messageLoc` → `methodNameLoc` * Extract `desugarBlock()` * Lift `block` desugaring up * Migrate `computeMethodCallLoc()` block param * Lift `block_given?` check up * Lift `isPrivateOk` local * Extract `desugarMethodCall()` Co-authored-by: Alexander Momchilov <alexander.momchilov@shopify.com> --------- Co-authored-by: Jesse Johnson <jesse.johnson@shopify.com> Co-authored-by: Thomas Marshall <thomas.marshall@shopify.com>
* Move getQuickfixEdits to lsp_helpers * prework: Expand concretizeIfAbstract We currently don't use this ShowOption for an `overridable` parent, so this change is a no-op. I thought about having a separate option, so that callers could opt into the overridable or the abstract behavior independently, but I don't think that there are any cases where you really need that level of granularity, so I left it controlled by a single boolean. I thought for a while about what a good name would be, but couldn't come up with anything more compelling then the verbose `concretizeIfAbstractOrOverridable`. Open to suggestions. * Add a ClassDefResponse to LSP QueryResponse One thing to note is that I opted not to implement the `retType` method on `ClassDefResponse`, because this isn't an expression. (Technically it is: it evaluates to the type of the last thing in the class body, but Sorbet doesn't model that. Sorbet lies and says that the return type is `NilClass`, but I figured it would be best to just raise). I looked at all the places where we are calling `retType` to make sure that this wouldn't cause problems, and ended up only needing to change `hover.cc` For my code action, I only had this match if your cursor is on the `declLoc`, not if it's inside the `loc` that spans the entire class body. I think that's something we could reconsider in the future, but I didn't want to do something where having an extra query response in the vector of query responses broke or changed the behavior of anything currently out there—limiting this to only `declLoc` limits the blast radius a bit. It also means that, for example, if your cursor is inside the class body on an empty line or a comment, you aren't presented with a lightbulb icon. I wanted to avoid having it be the case that there's _always_ a lightbulb icon following your cursor around. * Main implementation and tests * Add a quick screencast to the docs * fastmod getQuickfixEdits autocorrect2DocumentEdits * no-op: Remove core:: * Add test for Class.new * Handle case of no responses * clang-format * Fix the <root> declLoc problem * Fix CheckSize for ClassDefResponse
* Move getQuickfixEdits to lsp_helpers * prework: Expand concretizeIfAbstract We currently don't use this ShowOption for an `overridable` parent, so this change is a no-op. I thought about having a separate option, so that callers could opt into the overridable or the abstract behavior independently, but I don't think that there are any cases where you really need that level of granularity, so I left it controlled by a single boolean. I thought for a while about what a good name would be, but couldn't come up with anything more compelling then the verbose `concretizeIfAbstractOrOverridable`. Open to suggestions. * Add a ClassDefResponse to LSP QueryResponse One thing to note is that I opted not to implement the `retType` method on `ClassDefResponse`, because this isn't an expression. (Technically it is: it evaluates to the type of the last thing in the class body, but Sorbet doesn't model that. Sorbet lies and says that the return type is `NilClass`, but I figured it would be best to just raise). I looked at all the places where we are calling `retType` to make sure that this wouldn't cause problems, and ended up only needing to change `hover.cc` For my code action, I only had this match if your cursor is on the `declLoc`, not if it's inside the `loc` that spans the entire class body. I think that's something we could reconsider in the future, but I didn't want to do something where having an extra query response in the vector of query responses broke or changed the behavior of anything currently out there—limiting this to only `declLoc` limits the blast radius a bit. It also means that, for example, if your cursor is inside the class body on an empty line or a comment, you aren't presented with a lightbulb icon. I wanted to avoid having it be the case that there's _always_ a lightbulb icon following your cursor around. * Main implementation and tests * Add a quick screencast to the docs * fastmod getQuickfixEdits autocorrect2DocumentEdits * no-op: Remove core:: * Add test for Class.new * Handle case of no responses * clang-format * Fix the <root> declLoc problem * Fix CheckSize for ClassDefResponse * prework: Add declLoc to MethodDefResponse Previously, `termLoc` was `declLoc`, so I've also changed all the old usages of `termLoc` to `declLoc` to preserve the existing behavior. * prework: Only store the FileRef once to save space * prework: Add isAttrBestEffortUIOnly * Add completion item to override a method * Add a test * Add docs to website * Ban this test The file fails to parse, and prism doesn't handle those yet.
* prework: Add declLoc to MethodDefResponse Previously, `termLoc` was `declLoc`, so I've also changed all the old usages of `termLoc` to `declLoc` to preserve the existing behavior. * prework: Only store the FileRef once to save space * prework: Add isAttrBestEffortUIOnly * Add completion item to override a method * Add a test * Add docs to website * Ban this test The file fails to parse, and prism doesn't handle those yet.
This makes clangd work again. We recently upgraded to Bazel 7 (see 0e3ed53). In that release, they flipped the default of `--remote_download_outputs`, which broke our `clangd` integration depending on whether certain intermediate outputs were computed from scratch or downloaded from the cache. See <https://blog.bazel.build/2023/12/11/bazel-7-release.html#build-without-the-bytes-bwob>
* Create `include/` symlinks in LLVM toolchain This enables using the `clangd` that we fetch via Bazel, instead of a `clangd` on the user's PATH. A while back when we upgraded the `toolchains_llvm` (`bazel-toolchain`) library, it made this split between `llvm_toolchain_15_0_7` and `llvm_toolchain_15_0_7_llvm`, with symlinks from the former to the latter. That broke being able to use the `clangd` in the symlinked tools. This fixes it. The `clangd` troubleshooting page[^1] mentions that if you're struggling to get clangd to find stdlib headers, you might benefit from absolutely qualifying the path to `clangd`. That made me think that clangd is looking e.g. in the realpath of `external/llvm_toolchain_15_0_7` for an `include/` folder to find the system headers. But in our bazel sandbox, that external folder only had `bin` and `lib`, not `include`, so clangd failed to find them. The upstream change to `bazel-toolchain` creates those symlinks if `clangd` is one of the aliased tools, allowing the system headers to be found again. [1] <https://clangd.llvm.org/troubleshooting#cant-find-standard-library-headers-map-stdioh-etc> * Reverts ".vscode/settings.json: Use clangd from PATH (sorbet#9363)" Reverts 984c737
* Use name `**` for anonymous kwargs in method definitions This extracts some work done by @KaanOzkan in sorbet#9813 One difference between Prism and the existing Sorbet parser is that Prism does not provide unique identifiers for anonymous kwarg method parameters, which means that we either need to standardize the behavior or somehow maintain different sets of test expectations for the two parsers. In practice, having unique identifiers for anonymous kwarg nodes doesn't serve a purpose because you cannot declare more than one in the same scope, so this eliminates the unique identifiers and gives them all the `**` name. Co-authored-by: Kaan Ozkan <kaan.ozkan@shopify.com> * Add test for specifying the type of the values of an anonymous kwarg --------- Co-authored-by: Kaan Ozkan <kaan.ozkan@shopify.com>
* no-op: Remove unneeded branch * Remove unused checkForDynamicConstAssign * Remove more unused cases * Add a parameter * Remove template from translateConst
…orbet#9886) This reverts commit 788604a.
The behavior is different from the original parser which just throws the module away.
Co-authored-by: Thomas Marshall <thomas.marshall@shopify.com>
* Cache whether a package has sub-packages * Error for mixins or type members on a package namespace root This only applies for packages that have sub-packages, as that introduces a potential ordering issue if the sub-package is processed before the package containing the mixin or type member. * Add a test for the new error * Update existing tests * Better error messages * Add test packages to the reopen cli test * Improve error docs * Add a test for a package that's in the wrong place * Handle definitions for constants not owned by the current package * Update tests * Refactor canModify * Review feedback * Check superclasses as well * Update tests for superclass check * Fix testing of the test prefix * Add an enforce in directSubPackages * Switch to using onSymbol
sorbet#9887) * Convert keyword snippet to insertText ... and show what would happen to the tests if we did something naive when inserting the `insertText`. * Implement VS Code's `getWordRangeAtPosition` for handling `insertText` This lets our test harness approximate what VS Code does.
* Directly desugar lambdas * Make `DesugaredBlockArgument` move-only * Fix locations for numbered parameters
* Directly desugar `block_given?` * Add workaround for legacy behaviour
* Inline `node2TreeImplBody()` * Inline `node2TreeImpl()` * Inline `liftTopLevel()` into Translator * Elide avoidable `InsSeq` allocation * Clarify parameters
…#9890) When `RSpec.describe MyClass do` is rewritten, the synthetic class `<describe 'MyClass'>` was using `MyClass`'s source location, causing hover/go-to-def on `MyClass` to resolve to the synthetic class instead of the real constant. Fix by using a zero-length loc for the synthetic class name and preserving the original constant reference in the tree via InsSeq, matching the existing pattern used for `it` blocks. Committed-By-Agent: claude Co-authored-by: Claude <noreply@anthropic.com>
* Only run the resolver's global pass on the new symbols of a stratum * Add methods for iterating new symbols on GlobalState * Rename offsets to symbolOffsets * Fix a comment
* CLI flag * autocorrect for adding visibleTo * pre-work: refactor code for combining autocorrects * add an import and don't report an error when --update-visibilty-for is passed in * change regex for package name to * instead of + * aggregate missing visible_tos * add test case * update error reference * Update main/options/options.cc Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> * used named struct initializer * TODO about absl::c_lower_bound * early return if updateVisibilityFor_ is empty --------- Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
* Implement MethodTypeToParserNodePrism * Extract `unreachable` helper to `Exception.h` * Inline handling for `PM_KEYWORD_REST_PARAMETER_NODE` * Handle anonymous parameters in RBS rewriting --------- Co-authored-by: Emily Samp <emily.samp@shopify.com>
…m` (sorbet#9821) * Implement `SignatureTranslatorPrism` This is the entry point for logic that translates RBS types into Sorbet types * Implement `TypeParamsToparserNodesPrism` It's responsible for translating RBS generic type parameters into Sorbet's `type_member`
This prevents a crash in attempting to serialize a `nullptr` for the `location` of a `SymbolInformation`. It also makes the user experience better, because previously we would attempt to copy something like `Sorbet::Static::Private::<StubModule>` onto your clipboard. The lsp_test_runner test doesn't _quite_ exercise this crash, because we never actually call `toJSONValue`, but I've tested in VS Code directly and made sure that we don't crash anymore. I've also updated the test harness to check that `location` is not `nullptr` directly, which was the proximal cause of the crash.
* log the file names too * always print the files * Revert "always print the files" This reverts commit b03747e. * lint * use subspan and map_join * thousands seperator
amomchilov
force-pushed
the
Alex/desugar-csend
branch
from
74e3928 to
ed864f9
Compare
Author
|
Upstreaming in sorbet#9826 |
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
10 participants
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.
Motivation
Test plan
See included automated tests.