Skip to content

Infer block-pass symbols#793

Merged
castwide merged 11 commits into
masterfrom
infer-block-symbol
Mar 23, 2025
Merged

Infer block-pass symbols#793
castwide merged 11 commits into
masterfrom
infer-block-symbol

Conversation

@castwide

@castwide castwide commented Mar 17, 2025

Copy link
Copy Markdown
Owner

ref #777

Example:

array = [1, 2, 3]
array.map(&:to_s) # Inferred type is `Array<String>`

Requirements:

  • Chains recognize the block-pass symbol syntax
  • Modify Chain and Call to support Block and BlockSymbol links
  • Chain#infer resolves types from BlockSymbol and resolves the return type's generics

This is going to involve some non-trivial refactoring of source chains. Call links should be responsible for resolving block returns, so they need direct access to Block, BlockVariable, and BlockSymbol links passed into them. BlockSymbol and BlockVariable should not be included in Call#arguments. There should be a Call#block attribute instead. Resolution of the call's return type can then be moved to Call#resolve instead of Chain#infer.

@apiology

apiology commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

Thanks for pushing this up--glad to take this one off my list! I'll focus on getting apiology#1 ready to review.

Have you looked at the resolution code there?

Incidentally, if you can give me some rough definitions for 'resolve' 'infer' and 'probe', I'd be happy to send a doc PR for the next person. I think I know but suspect I'll learn something if it comes out in your words.

@castwide

castwide commented Mar 17, 2025

Copy link
Copy Markdown
Owner Author

Thanks for pushing this up--glad to take this one off my list! I'll focus on getting apiology#1 ready to review.

Have you looked at the resolution code there?

No, I hadn't seen it. That'll be important to the test that fails array.map(&.to_s). I came close to climbing down that rabbit hole, so thanks for bringing your work on it to my attention.

In the meantime, I can continue work on block chaining. (I'll try not to make too many bitcoin puns.)

Incidentally, if you can give me some rough definitions for 'resolve' 'infer' and 'probe', I'd be happy to send a doc PR for the next person. I think I know but suspect I'll learn something if it comes out in your words.

Chain#infer is the method that determines a code snippet's type through static analysis of a node chain. Clip#infer depends on Chain#infer. In method and variable pins, #probe uses Clip#infer under the hood; other pins fall back to #typify.

Chain::Link#resolve returns an array of pins that can be probed for a return type. Chain#define calls #resolve on each of its links in order, passing the first resolved pin of the last link to the next link, and returns the last link's results.

Here's an example. SourceChainer parses 'string'.length.abs into a chain with three links: a Literal (String), a Call to length, and a Call to abs. Type inference on the chain works like this:

  • Chain#infer calls #define.
  • Chain#define calls #resolve on each of its links.
  • Link 1 (String) returns an anonymous pin with a String return type.
  • Link 2 (length) takes the String pin and searches for method pins named length in the String namespace.
  • Link 3 (abs) takes the String#length pin and searches for method pins named abs in the Integer namespace.
  • Chain#infer takes the last array of pins and returns the first defined return type it finds. It'll try to #typify the pin first, which checks for documented types. If #typify returns undefined, it tries #probe.

Hope that makes sense.

@castwide
castwide force-pushed the infer-block-symbol branch 2 times, most recently from 4a4985d to 21f1c7f Compare March 17, 2025 18:20
@apiology

Copy link
Copy Markdown
Contributor

#794 is ready for review if you want to use bits of it

@castwide
castwide force-pushed the infer-block-symbol branch from 9a5f3ae to df78072 Compare March 19, 2025 23:47
@castwide
castwide force-pushed the infer-block-symbol branch from df78072 to dd9d68d Compare March 20, 2025 00:00
@castwide

castwide commented Mar 21, 2025

Copy link
Copy Markdown
Owner Author

This PR might have exposed a gap in the generics feature.

The currently failing test involves Array#map:

array = [100] # Correctly inferred as [Array<Integer>]
array.map(&:to_s) # Should be [Array<String>] but inferred as [Array] instead

The Array#map return type's generic is specific to the method, not the class. In RBS, Enumerable#map is defined like this:

def map: [U] () { (Elem arg0) -> U } -> ::Array[U]
       | () -> ::Enumerator[Elem, ::Array[untyped]]

I think The YARD equivalent should look something like this:

# @generic U
# @yieldparam [generic<Elem>] (`@generic Elem` is defined on `Enumerable`)
# @yieldreturn [generic<U>]
# @return [Enumerator<generic<Elem>>, Array<generic<U>>]
def map; end

@apiology

apiology commented Mar 21, 2025

Copy link
Copy Markdown
Contributor

To be pedantic, "method" generics appear to be defined on the signature-level - though the docs in the RBS gem get a little loose with terminology.

e.g. Enumerable#find:

  def find: () { (Elem) -> boolish } -> Elem?
          | () -> ::Enumerator[Elem, Elem?]
          | [T] (_NotFound[T] ifnone) { (Elem) -> boolish } -> (Elem | T)
          | [T] (_NotFound[T] ifnone) -> ::Enumerator[Elem, Elem | T]

@apiology

Copy link
Copy Markdown
Contributor
# @generic U
# @yieldparam [generic<Elem>] (`@generic Elem` is defined on `Enumerable`)
# @yieldreturn [generic<U>]
# @return [Enumerator<generic<Elem>>, Array<generic<U>>]
def map; end

I think we really need two signatures like RBS has and to resolve between them based on whether a block is passed in to map(). The former one, with the block, actually does return an Array<generic>:

def map: [U] () { (Elem arg0) -> U } -> ::Array[U]
foo = some_enumerable.map { |s| s.length } # foo is an Array<Integer>

Array<Integer> can be resolved by inferring the return type of the block, then adding that info into the call to Pin::Signature#resolve_generics_from_context for it to resolve the signature.

@apiology

apiology commented Mar 21, 2025

Copy link
Copy Markdown
Contributor

For the block-less signature:

       | () -> ::Enumerator[Elem, ::Array[untyped]]

Note that this is an Enumerator, which has some special powers above and beyond being an Enumerable. I'm a little fuzzy on it myself, but here's an example use from the rbs docs:

# This allows you to chain Enumerators together.  For example, you can map a
# list's elements to strings containing the index and the element as a string
# via:

puts %w[foo bar baz].map.with_index { |w, i| "#{i}:#{w}" } # Types as Array<String> because .with_index() looks at the block return type, not .map
# => ["0:foo", "1:bar", "2:baz"]

The reason that the second type parameter to Enumerator is untyped in this signature is because we can't know what the user is going to produce in their block at the point where we call .map() in this case.

The second type parameter of Enumerator seems to relate to what the return value of .with_index {} and .each {} are - which isn't typically used by anyone I would think and we don't need to sweat. Enumerator even defaults it to void:

class Enumerator[unchecked out Elem, out Return = void] < Object

@apiology

Copy link
Copy Markdown
Contributor

In general I wonder if we can remove Pin::Method#parameters and #block entirely and instead force code to look at each signature individually. I haven't tried, so I know that's just big talk right now :). If there are only method-level YARD annotations, we'd just map those into the first Pin::Signature.

But agreed that the combined method-level annotations look could like the above if we want YARD's method-level tags to keep to the model of the union of all signatures tags.

Comment thread lib/solargraph/source/chain.rb Outdated
ComplexType.parse(*sorted)

if possibles.first.map(&:name).include?('Enumerator') && links.last.is_a?(Call) && links.last&.arguments&.first&.links&.first.is_a?(BlockSymbol)
ComplexType.parse(possibles.first.items.find { |sub| sub.name != 'Enumerator' }.to_s)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line a no-op? It's not getting assigned to anything

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of line 174 will get returned from the method, but I'm likely to remove the if condition anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on - I'm probably getting lost in indention is all.

@castwide

castwide commented Mar 21, 2025

Copy link
Copy Markdown
Owner Author

@apiology Good points about signatures and enumerators. In YARD, it should probably look more like this:

# @generic U
# @overload map
#   @return [Enumerator<generic<Elem>, generic<U>>
# @overload map(&)
#   @yieldparam [generic<Elem>]
#   @yieldreturn [generic<U>]
#   @return [Array<generic<U>>]
def map; end

In general I wonder if we can remove Pin::Method#parameters and #block entirely and instead force code to look at each signature individually. I haven't tried, so I know that's just big talk right now :). If there are only method-level YARD annotations, we'd just map those into the first Pin::Signature.

Some of that functionality is already there. When method pins are generated from YARD, the first signature is generated from the method-level YARD annotations, and the rest are generated from @overload tags. It might make sense to put the method-level signature last instead; presumably, @overload signatures will have more refined specs and should therefore have higher precedence.

@apiology

Copy link
Copy Markdown
Contributor

I should probably read up on the Yard docs on overloads and make sure I understand their semantics before I open my mouth on that topic :) My thoughts were mainly around our internal representation for sure.

@castwide

Copy link
Copy Markdown
Owner Author

I should probably read up on the Yard docs on overloads and make sure I understand their semantics before I open my mouth on that topic :) My thoughts were mainly around our internal representation for sure.

Yeah, I probably don't need to worry about making the YARD version work yet. For Enumerable in particular, proper inference based on RBS signatures is definitely more urgent. Imagining the YARD representation helps me wrap my head around the internal representation, but changing @overload processing can be deferred to a separate issue.

@apiology

Copy link
Copy Markdown
Contributor

Array can be resolved by inferring the return type of the block, then adding that info into the call to Pin::Signature#resolve_generics_from_context for it to resolve the signature.

I took a shot at implementing this in #804 - let me know if this seems at least directionally correct

@castwide
castwide marked this pull request as ready for review March 23, 2025 11:21
@castwide
castwide merged commit a8c4b23 into master Mar 23, 2025
apiology pushed a commit to apiology/solargraph that referenced this pull request Apr 6, 2025
* BlockSymbol link

* First iteration of block-pass symbol resolution

* Chain::Call#block

* Superfluous block tracking

* Refactor NodeChainer

* Fix chain block check

* Sync with castwide#780

* BlockSymbol link

* First iteration of block-pass symbol resolution

* Change spec to test Call#block

* Resolve generics from BlockSymbol
castwide added a commit that referenced this pull request Apr 15, 2025
* Retire more RubyVM-specific code

* Add basic plugin integration testing (#798)

* Add basic plugin integration testing

Add a GitHub Actions workflow to run very basic smoke tests
for solargraph-rails and solargraph rspec (typechecking and then
running solargraph's specs with it installed and configured)

Also:

* Document solargraph-rspec as a plugin in README.md

* Add solargraph-rspec to .Gemfile as well

* Fix YAML syntax

* Fix GitHub workflow syntax

* Fix GitHub workflow syntax

* Fix GitHub workflow syntax

* Fix GitHub workflow syntax

* Switch yq syntax for the *other* yq, cache apt installation

* Switch yq syntax for the *other* yq, cache apt installation

* Restore Legacy module for solargraph-rails backwards compatibility

* Infer block-pass symbols (#793)

* BlockSymbol link

* First iteration of block-pass symbol resolution

* Chain::Call#block

* Superfluous block tracking

* Refactor NodeChainer

* Fix chain block check

* Sync with #780

* BlockSymbol link

* First iteration of block-pass symbol resolution

* Change spec to test Call#block

* Resolve generics from BlockSymbol

* More annotations in Solargraph code (#801)

* More annotations in Solargraph code

* More annotations from strict typechecking

* More internal annotations

* Add some method overloads

* Add #to_rbs methods to pins, use for better .inspect() output (#789)

* Add #to_rbs methods to pins, use for better .inspect() output

* Add inspect() to Chain::Link, to_rbs to Pin::Namespace

* Fix typo

* Show variable assignments in #desc

* Method generics resolution via argument types (#794)

* Improve block handling in signature selection

Fixes #777

* Resolve method generics

* Track generics in method signatures from RBS and YARD

* Erase method generics that couldn't be resolved

* Refactor Pin#resolve_types to use Pin#transform_types

* Fix issues with UniqueType#transform()

* Cleanups

* Move to keyword parameters for resolved_generic_values

* Add type erasure, method pin rewrite code

* Populate Method parameter/return_type/block from signature

* Mark completed TODOs

* Ruby 3.0 compatibility

* Add erase_generics() to ComplexType::TypeMethods

* TODO -> @todo

* Resolve some TODOs

* Resolve TODOs

* Revert accidental change

* Bring back spec

* Provide full tag to get_method_stack() for better inference

* Fix a missed spot referring to allowlist

* Ensure Signature#generics is not nil

* Various #to_rbs and #inspect improvements

* Clean up code

* Drop accidental addition

* Improve RBS generation from UniqueType

* Use rooted names when generating RBS when available

* Drop bad documentation

* Remove deprecated commands (#790)

* Cache command

* Add :if support to NodeChainer for if statements as lvalues (#805)

* Add :if support to NodeChainer for if statements as lvalues

* Add :if support to NodeChainer for if statements as lvalues

* Fix ApiMap::Cache (#806)

* Fix ApiMap::Cache

* Refactoring

* Gems command

* Various DocMap improvements (#807)

* Retire dead DocMap-related code, restore 'solargraph uncache'

* Best-effort work to support bundler's 'path:' directive in Gemfiles

* Restore support for 'solargraph uncache'

* Clarify names in DocMap

* Tell Thor to exit with status 1 on error, inc. deprecated command

* Clarify shell deprecation messages

* Send deprecation errors to stderr

* Fix merge

* Async gem caching (#809)

* Various regression and future-feature specs (#800)

* Various regression and future-feature specs

* Merge remote-tracking branch 'origin/master' into more_specs

* Another batch of specs

* Drop duplicated spec

* Various regression and future-feature specs

* Various regression and future-feature specs

* Resolve a small issue found on merge

* Fix pre-mature marking of spec as ready

* Unify method generics resolution (#810)

* Map mixins from RBS (#808)

* Map mixins from RBS

* DRY mixin processing

* Fix issue with wrong signature selection by call with block node (#815)

We were picking the wrong signature for Enumerator#select and
Enumerator#find because Chain::Call didn't have access to the block
that was passed in and didn't think the arity matched as a result.

* Keep gem pins in memory (#811)

* Keep gem pins in memory

* DocMap warns on unresolved paths

* Fic cache errors

* Keep stdlib pins in memory

* Move logs for pins in memory from info to debug

* Use previous stdlib memory cache

* Use cursor node in clip cache index

* Changelog

* Add more internal type annotations required by strict typechecking (#814)

* Add more internal type annotations required by strict typechecking

* Add more internal type annotations required by strict typechecking

* Fix types in existing annotations

* Fix names in existing annotations

* Add more internal type annotations required by strict typechecking

* Add more internal type annotations required by strict typechecking

* Disable ApiMap clip cache (#817)

* Refactor gems command (#816)

* Use return type of literal blocks in inference (#818)

* Use simple return type of literal blocks in inference

Also:
* Document block symbol handling issue
* Add failing spec for less trivial literal block inference

* Mark spec as working after merging in latest master

* Add Thread::Mutex#synchronize spec just to be sure

* Fix an internal return type annotation

* Insert Module methods (#820)

* ApiMap#get_methods includes Module

* Redundant code

* Another test

* Fix kwoptarg string in Parameter#to_rbs (#822)

* Revise documentation formatting (#823)

* Revise documentation formatting

* Ruby 3.0 compatibility

* Changelog

* DocMap logging (#825)

* Clean up DocMap logging

* Remove stale requires and autoloads

* Redundant `require 'set'`

* Test redundant require

* Ignore test in Ruby < 3.1.1

* Stub test

* Require set at root and suppress warning

* Remove workaround for now-fixed ComplexType.parse limitations (#827)

Parser bug was fixed in #775

* Add annotations needed for strict typechecking of solargraph code (#828)

* Rescue exceptions in docstring parsing (#830)

* Update README

* Reject nil requires in live code (#831)

* DocMap rejects nil requires

* Handle empty requires

* RbsMap adds mixins to current namespace (#832)

* RbsMap adds mixins to current namespace

* Smoke test

* Release 0.53.1

* Fix a self-type-related false-positive in strict typechecking (#834)

* DocMap fetches gem dependencies (#835)

* Use configured command path to spawn solargraph processes (#837)

* Release 0.53.2

* Remove redundant core fills (#824)

* Remove Errno core fills

* Retire CoreSigns

* Retire MISSING core fills

* Remove implicit .new pins

* Stale specs

* Switch Class#new to [Caller]#initialize for arity checks

* Remove implicit .new pins from source maps

* Resolve self type in variable assignments (#839)

* Eliminate splat-related false-alarms in strict typechecking (#840)

* Dynamic block binding with yieldreceiver (#842)

* Rebind with yieldreceiver

* Unused rebindable_method_names

* Refactor Block#binder_or_nil

* Resolve generics by descending through context type (#847)

* Release 0.53.3

* [regression] Restore 'Unresolved call' typecheck for stdlib objects (#849)

The 'Unresolved call to' typecheck suppresses warnings for calls
against non-RBS-origin pins.  However, the labeling assigning :rbs as
the source of pins from conversions.rb was recently removed as part of
an unrelated cleanup.

* Lazy dynamic rebinding (#851)

* Lazy dynamic rebinding

* Stub failing test

* Move rebinding to Clip

* Restore fill for Class#allocate (#848)

* [regression] Ensure YardMap gems have return type for Class<T>.new (#850)

* [regression] Ensure YardMap gems have return type for Class<T>.new

* Handle self return-type-setting at lower level

* Fix bug where signature return type was based only on annotation

* Create implicit .new pins in namespace method queries (#853)

* Create implicit .new pins in namespace method queries

* Comment for Host#locate_pins exception

* Release 0.53.3

* Use logger in Solargraph::YardMap::Mapper spec

* Add support for simple block argument destructuring (#821)

* Add support for simple block argument destructuring

* Add spec for Array<Array(String, Integer)>#each

* Internal typechecking-driven annotations and code changes (#838)

* Internal typechecking-driven annotations and code changes

* Add @todo for predicate method return type issue

* Internal typechecking-driven annotations and code changes

* Benchmark the typecheck command (#852)

* Enable passing tests (#854)

* Send Gem Caching Progress Notifications to LSP Clients (#855)

* Send gem caching notifications

* LanguageServer::Progress

* DRY progress notifications

* Refactor

* added failing tests (#573)

* [breaking] Fix more complex_type_spec.rb cases (#813)

* [breaking] Fix more complex_type_spec.rb cases

* Consolidate the code that generates substrings to a single place
* Clean up more paths to pass through 'rooted' flag unchanged
* [breaking] Drop special-case interpretation of, e.g., `Array<(String)>` as
  'one element tuple' for future consistency with
  https://yardoc.org/types

  * (String) is a one element tuple in https://yardoc.org/types
  * <String> is an array of zero or more Strings in https://yardoc.org/types
  * Array<(String)> could be an Array of one-element tuples or a
    one element tuple.  https://yardoc.org/types treats it
    as the former.
  * Array<(String), Integer> is not ambiguous if we accept
    (String) as a tuple type, but not currently understood
    by Solargraph.

* Close out complex_type_spec @todos for better RBS output

* Simplify qualify() with transform()

* Resolve merge issue

* Mass assignment support - e.g., a, b = ['1', '2'] (#843)

* Memoize result of Chain#infer (#857)

* Memoize result of Chain#infer

* Add links to cache key

* Add node.location separately to cache key

https://github.com/whitequark/ast/blob/master/lib/ast/node.rb

https://github.com/whitequark/parser/blob/05b88aa0468ce29dfa21e1aff2f0506b1d5d82e1/lib/parser/ast/node.rb#L17

Parser::AST adds 'location' in Parser::AST::Node to its superclass
AST::Node, but doesn't overwrite the #hash and #eql? methods

* Fall back to using links.map(&:word)

* Implement eql? and hash for ApiMap for cache coherency

* Remove @cache from ApiMap#equality_fields

* Remove clip caches (#860)

* Add rbs_collection to maps (#858)

* Fix flaky rename specs (#869)

* Ignore malformed mixins and overloads (#862)

* Ignore malformed mixins

* Ignore malformed overload tags

* Add internal type annotations needed for strict-level typechecking (#865)

* Add internal type annotations needed for strict-level typechecking

* Fix annotations

* Add internal type annotations needed for strict-level typechecking

* Fix flaky rename specs

* Move to untyped as Hash value

---------

Co-authored-by: Fred Snyder <fsnyder@castwide.com>

* Drop Parser::ParserGem::ClassMethods#returns_from_node (#866)

* Refactor TypeChecker#argument_problems_for for type safety (#867)

* Refactor TypeChecker#argument_problems_for for type safety

This should put it in a state that it can pass type checking once we
have another feature or two

* Editor compatibility hack

* Fix class name

* Fix flaky rename specs

---------

Co-authored-by: Fred Snyder <fsnyder@castwide.com>

* Specify more type behavior for variable reassignment (#863)

* One-step source synchronization (#871)

* One-step source synchronization

* Synchronize libraries

* Fix progress notification timing (#873)

* Show cache progress in shell commands (#874)

* Show cache progress in shell commands

* Typecheck error

---------

Co-authored-by: Fred Snyder <fsnyder@castwide.com>
Co-authored-by: BonitaTerror <39413300+BonitaTerror@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants