Skip to content

Tags: sorbet/sorbet

Tags

0.6.12632.20251008110159-445206eb8

Toggle 0.6.12632.20251008110159-445206eb8's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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>

0.6.12631.20251008103647-588bfab4d

Toggle 0.6.12631.20251008103647-588bfab4d's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12630.20251007155449-57cd2cf49

Toggle 0.6.12630.20251007155449-57cd2cf49's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.

0.6.12629.20251007115822-694b2a45c

Toggle 0.6.12629.20251007115822-694b2a45c's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12628.20251007105314-3a3367f6a

Toggle 0.6.12628.20251007105314-3a3367f6a's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.

0.6.12627.20251003165043-6f7c5f726

Toggle 0.6.12627.20251003165043-6f7c5f726's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12626.20251003161800-c27a37bc7

Toggle 0.6.12626.20251003161800-c27a37bc7's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12625.20251003152005-26a81376c

Toggle 0.6.12625.20251003152005-26a81376c's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12624.20251003145818-0c5377a2c

Toggle 0.6.12624.20251003145818-0c5377a2c's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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

0.6.12623.20251003125806-f0faea574

Toggle 0.6.12623.20251003125806-f0faea574's commit message

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Add another test (#9431)

Requested by @neilparikh in #9419