Desugar PM_SUPER_NODE#832
Closed
amomchilov wants to merge 10 commits intoAlex/extract-desugarMethodCallfrom
Closed
Desugar PM_SUPER_NODE#832amomchilov wants to merge 10 commits intoAlex/extract-desugarMethodCallfrom
PM_SUPER_NODE#832amomchilov wants to merge 10 commits intoAlex/extract-desugarMethodCallfrom
Conversation
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
* 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>
amomchilov
force-pushed
the
Alex/desugar-super
branch
3 times, most recently
from
c45458c to
1b4649f
Compare
* 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.
amomchilov
force-pushed
the
Alex/desugar-super
branch
from
1b4649f to
104ad38
Compare
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
4 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.