Tags: sorbet/sorbet
Tags
Add a bunch of comments about types (#9441) * Add a bunch of comments about types * Update core/Types.h Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> --------- Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
Only query for multiple symbols at a time (#9438) * prework: Solve MoveMethod edge case differently I want to remove `originalSymbol`. It was added for this check. There are tests that fail if I comment this check out: //test:test_LSPTests/testdata/lsp/code_actions/move_method/class_func //test:test_LSPTests/testdata/lsp/code_actions/move_method/either Looking at the failures, I think we can solve this by doing a simpler check that doesn't require knowing the `originalSymbol`. * no-op: Remove originalSymbol Now that we're not using it in MoveMethod, we can get rid of this. Also, it's a better interface: not all rename requests come from renaming symbols, so it was not actually possible to rely on the symbol existing (e.g., in the LocalRenamer implementation). * Only one typechecker operation when doing renames * no-op: clang-format * Draw the rest of the own Makes it so that you can only query for multiple symbols There are actually not that many places that are only searching for one symbol. * Also make only one query per send Previously, we would make one find-all-references request per `DispatchComponent`. Now, we only make one. * no-op: Get rid of now-unused priorRefs arg * no-op: Remove unused variable * no-op: Some Renamer refactors - No need to use `shared_ptr`: the ownership is clear - Removes `addSymbol` from public interface. Not all renamers deal with symbols (e.g., the LocalRenamer), so let's move this into the constructor for each renamer. - As a result, don't need to take symbol in `getEdits` function, because we can assume that the symbol queue is populated. * Make MethodRenamer operate on multiple symbols * Renaming one symbol might enqueue another symbol
Rename `instantiate` so it's not overloaded (#9440) * no-op: Rename instantiate → instantiateTypeVars There are two overloads of instantiate, and every time I need to look at the code that powers generics, I get tripped up because I'm looking at the wrong one. * no-op: Remove unused include * no-op: Rename instantiate → instantiateLambdaParams c.f. previous * no-op: Rename approximate → approximateTypeVars I was never confused by this one, but I figured for consistency's sake I would rename this one too. * no-op: Remove direct calls to Types::instantiateLambdaParams There is now only one call to this: `Types::resultTypeAsSeenFrom`. That makes it more clear that people are not meant to call `instantiateLambdaParams` directly.
TypeArgument → TypeParameter (#9439) * fastmoda '\bTypeArgumentRef\b' 'TypeParameterRef' * fastmoda '\bgetOrCreateTypeArguments\b' 'getOrCreateTypeParameters' * fastmoda '\bexistingTypeArguments\b' 'existingTypeParameters' * textDocument/rename: typeArgs → typeParams * fastmoda '\benterTypeArgument\b' 'enterTypeParameter' * fastmoda '\bKind::TypeArgument\b' 'Kind::TypeParameter' * manual: Kind definition: TypeArgument -> TypeParameter * fastmoda '\btypeArguments\(\)' 'typeParameters()' * fastmoda '\bisTypeArgument\(\)' 'isTypeParameter()' * textDocument/rename: isTypeArgument → isTypeParameter * fastmoda '\btypeArgumentsUsed\b' 'typeParametersUsed' * manual: GlobalState::{typeArguments → typeParameters} * fastmoda '\basTypeArgumentRef\b' 'asTypeParameterRef' * fastmoda '\bnoTypeArgument\(\)' 'noTypeParameter()' * fastmoda '\bNoTypeArgument\b' 'NoTypeParameter' * fastmoda '\bTYPEARGUMENT\b' 'TYPEPARAMETER' * fastmoda '\btypeArgumentIndex\(\)' 'typeParameterIndex()' * fastmoda 'TypeArgumentTableCapacity\b' 'TypeParameterTableCapacity' * manual: A comment * fastmoda '\btypeArgumentsSize\b' 'typeParametersSize' * fastmoda '\btodoTypeArgument\(\)' 'todoTypeParameter()' * textDocument/rename: typeArgumentsSizeScaled → typeParametersSizeScaled * fastmoda '\bTodoTypeArgument\b' 'TodoTypeParameter' * textDocument/rename: typeArgument → typeParameter * manual: Fix an ENFORCE message * textDocument/rename: typeArgumentSize → typeParameterSize * textDocument/rename: typeArgument → typeParameter * textDocument/rename: methodTypeArguments → methodTypeParameters * textDocument/rename: typeArgument → typeParameter * textDocument/rename: typeArgument → typeParameter * manual: Fix a comment * textDocument/rename: assorted typeArg * fastmoda '\bTypeArgSpec\b' 'TypeParamSpec' * textDocument/rename: typeArgs → typeParams * fastmoda '\benterTypeArgByName\b' 'enterTypeParamByName' * fastmoda '\bfindTypeArgByName\b' 'findTypeParamByName' * clang-format
Make Symbol queries search for multiple symbols (#9437) * fixup: Add `#include "core/SymbolRef.h"` It looks like this only compiled because this file was included in two places, and those places already had an `#include` which defined `SymbolRef` earlier in the list of `#include`'s. I noticed this when I opened the file in my IDE and got a bunch of errors for it. * prework: Tidy up the Query interface Don't use references when the types are trivially copyable `Ref` classes. Use `MethodRef` in place of `SymbolRef` wherever relevant. * prework: Add a CheckSize assertion * prework: Use std::variant for `lsp::Query` We were kind of already doing this by way of a `Kind` tag and the overloaded meanings of each field. Note that this doesn't increase the size of the `lsp::Query` class, though this is not particularl memory sensitive. * no-op: Convert private methods to helper funcitons I wanted to modify this, and was surprised to see that I had to edit the header. This function doesn't use any of the task state. * Make Symbol queries search for multiple symbols There are a bunch of places where we're doing multiple typechecker operations to search for mutiple SymbolRefs. Instead of doing that, let's do one typechecker operation, but with a query that allows matching against multiple symbols.
lsp: Report location-less error info in Diagnostic message (#9299) * lsp: Push ErrorSection's header into first RelatedInformation This translates to less noise in the language client. There is an impedance mismatch in the structure that Sorbet uses to report errors and the structure that the LSP spec needs. Because the two structures are not equivalent, there will always be some wonkiness. @damolina-stripe reported a particularly bad case: ```json { relatedInformation: [ {location: ..., message: ""} {location: ..., message: "Defined here"} {location: ..., message: "Note:"} {location: ..., message: "Please run gen-packages to fix this"} ] } ``` We could attempt to solve the problem for related information entries that are exactly "defined here" or "note" but it would be kind of heuristic-y and weird. In an ideal world, we might update the data structure that we use for reporting errors to the LSP client to make it track information that would instruct how ErrorReporter is meant to translate Sorbet's errors into LSP diagnostics. Another thing we might want to do in the future is to append any ErrorLines without location information (e.g., `Note:` lines) into the Diagnostic's top-level message, instead of into the RelatedInformation struct (so that we don't have to pick an arbitrary location). For the time being, at least we can cut down on the number of things in the RelatedInformation array by flushing section headers in front of the first ErrorLine's information. * Update rec test * lsp: Report location-less error info in Diagnostic message * wip * stog //test/lsp:update_incremental-lsp-changes
completion: Put the `(|)` and `{|}` back into the snippet (#9432) * completion: Put the `(|)` and `{|}` back into the snippet Follow up to #9425. After typing out a bunch of snippets, I'm noticing how much muscle memory I had for the `()` getting put into the snippet for me. I noticed that IntelliJ also had this behavior, so I think it's not too wild for Sorbet to also do it. TypeScript doesn't do it, but JavaScript allows passing around first-class objects, while if you want to do that in Ruby you're going to have to do something else entirely (`x.method(:foo)` vs `x.foo`). I've chosen to only insert the `()` for methods that have required parameters, because the overwhelming style in the Ruby community is to omit trailing parens for nullary method calls (despite my petitions to change that). While I was here, I figured that if the method had no required arguments, but did have a required block, then we should also go ahead and put the user's cursor inside a `{|}`. I didn't want to combine those (put both the `(|) {|}` in one snippet) because then we end up with the same problem we had before, which is that the presence of a snippet that has multiple tabstops prevents completion requests from firing. * More tests that change
Go to Definition for keyword argument (#9428) * prework: Fix the test These `hover-line` assertions were not testing anything because they forgot the `:`. Turns out, they were also asserting the wrong thing. Luckily the actual behavior was correct. * Go to Definition for keyword argument Some things to note about the test: - We have to use the `not-def-of-self` assertion so that we don't attempt to make a definition and references request on that `def` assertion. Rather, it's just the target of a `go-to-def-special` assertion. - It can't be a def of self, because we've wired up find all references on a method parameter to mean "references of the local variable inside the current method," so it can't also mean "show me all references of this keyword argument at callsites" * Tack on type definition support * Code review suggestion * Grammar fixups
First-class KeywordArgResponse handling (#9427) * no-op: Use exhaustive visitor in some helpers I think that eventually I want to replace all the usages in the various main/lsp/requests/ implementations, but that's going to have to wait for some other time, I'm in a rush. * no-op: Remove getTypeAndOrigins helper It was not used widely enough to justify itself. * First-class KeywordArgResponse handling Handled by calls.cc, so that we don't have reimplement that ad hoc in hover.cc. Will make it easier if we want to make use of this in other requests. It should be pretty easy to use this to implement support for go-to-def and go-to-type-def. If we wanted to add support for references, we would need to also build support for a `PARAM_INFO` kind of `lsp::Query`. Right now, references only lets you find references of symbols, unclear whether that's something worth doing right now. * One test changes I thought about this for a while. What's happening is that the loc of the keyword symbol and the synthetic variable of the value are different: example(account:) # ^^^^^^^ the `:account` symbol # ^^^^^^^^ the `account` variable ~~These locs are load bearing.~~ Not sure whether these locs are load bearing at the moment, I had done some spelunking but now I think that it might not matter if we wanted to change them to make them equal. Since the `:account` symbol is a more narrow loc, it gets sorted to appear lower in the `queryResponses`, and thus we hit the `isKeywordArg` case instead of hitting the `isIdent` case. I thought about ways to recover this (change the loc of the symbol as mentioned above, or do some more "scan back over the `queryResponses`" logic like we used to be doing before this PR). But then I thought about it, and I'm not even sure what the user would want to see here. It's possible that the answer is "both," it's possible that the answer is just the keyword param info. And since it's ambiguous, I think that asking people to manually un-pun the keyword arg to get the one that they're after is not that bad. Anyways, out of convenience, I've opted to just let the behavior change here. We could change it back if that ends up being people's preference. * no-op: Also make this function exhaustive * Change loc of punned keyword args It looks like the original "drop the trailing :" came from Dmitry's change to overhaul how we store arguments in the symbol table: b7d0554 The original PR is Stripe-internal: http://go/si/sorbet/pull/1888 During that PR, locations of keyword parameters became load bearing in certain cases, because Sorbet would read the original source of the file to show parameter names in error messages sometimes. It looks like that helper was then cargo culted to implement punned keyword arguments. Sorbet's `pair_keyword` builder method was used in multiple places, which included parameter lists, argument lists, hash lists, etc. But this `pair_label` builder method is only used outside of method parameter lists, so it cannot affect that use case, and we can go back to the original location information here. It's possible that we want to refactor things so that `pair_keyword` uses this same loc that `pair_label` uses when it's not being used in a parameter list, but it doesn't look like we have any tests that require us to do that at the moment, so I'm going to punt on it. * Revert "One test changes" This reverts commit 371494e. * Update CheckSize
Add another test (#9431) Requested by @neilparikh in #9419
PreviousNext