Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type more things #24

Merged
merged 24 commits into from
Jul 11, 2024
Merged

Type more things #24

merged 24 commits into from
Jul 11, 2024

Conversation

lloeki
Copy link
Member

@lloeki lloeki commented Jun 4, 2024

This is an attempt to fix as much typing issues as possible, ideally all.

@@ -66,7 +66,7 @@ module Graft

def disabled?: (String key) -> bool

module Prepend
module Prepend : HookPoint
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

Does it mean HookPoint class will include this module?

Copy link
Member Author

@lloeki lloeki Jun 5, 2024

Choose a reason for hiding this comment

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

This self type parameter constraints the Prepend module to only be mixed in HookPoint, therefore Prepend can assume having access to HookPoint methods.

This happens automatically in HookPoint#initialize:

    def initialize(hook_point, strategy = DEFAULT_STRATEGY)
      @klass_name, @method_kind, @method_name = HookPoint.parse(hook_point)
      @strategy = strategy
      extend HookPoint.strategy_module(strategy)
    end

The actual implementation of a given strategy is composed into HookPoint, which only provides a uniform interface largely independent of the strategy.

@lloeki lloeki force-pushed the type-more-things branch from 5689606 to 822b9b6 Compare July 3, 2024 19:45
lloeki added 7 commits July 10, 2024 11:28
Steep does static type checking, so:

a. sees the conditional `def` twice
b. can't have conditional definitions
c. uses one definition for both

With `define_method`, Steep considers the definition dynamic and does
not try to check the method.
Two main cases:

- `attr_*` needs `@dynamic`: soutaro/steep#1036
- three `define_method` because of the `RUBY_VERSION` condition
This `self` constraint declares what the module can extend, and
therefore makes the corresponding `self` methods visible inside these
modules.
When using `case` steep types the unreachable `else` as `bot`, which
ends up having no `#inspect`.

Theoretically we would remove the unreachable fallback (`else` or simply
code after `end`) but out of caution we want this safety check.

Workaround it with a few `if`.
@lloeki lloeki force-pushed the type-more-things branch from 822b9b6 to ca621f9 Compare July 10, 2024 09:50
@lloeki lloeki marked this pull request as ready for review July 11, 2024 10:51
@lloeki lloeki requested a review from a team as a code owner July 11, 2024 10:51
@lloeki
Copy link
Member Author

lloeki commented Jul 11, 2024

And now typing passes!

The trickier bits were mostly because:

  • steep bugs or limitations (a few issues remain to be opened upstream)
  • Ruby 2.x vs 3.x kwargs handling
  • Simply the fact that graft does some very dynamic metaprogramming, which is inherently tricky to type

@lloeki lloeki merged commit b23c747 into main Jul 11, 2024
17 checks passed
@lloeki lloeki deleted the type-more-things branch July 11, 2024 10:53
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