Remove use of hidden-definitions in favor of Tapioca compilers #16586

dduugg · 2024-02-05T06:10:17Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Resolves #16557:

hidden-definitions is slow, deprecated, and #16281. Let's replace it with Tapioca DSL compilers and RBI files where needed. (It currently masks 160 type errors, though half of them should resolve when #16540 merges.)

This also reduces the time to run brew typecheck --update-all by > 30% (5m7s → 3m31s on an M1 Pro, n=4)

MikeMcQuaid

Looks good so far!

Library/Homebrew/Gemfile

Library/Homebrew/rubocops/cask/extend/node.rb

Library/Homebrew/rubocops/cask/extend/node.rbi

Library/Homebrew/sorbet/rbi/dsl/.gitattributes

dduugg · 2024-02-09T20:09:31Z

Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit/assert_statements.rbi

+# This is an autogenerated file for dynamic methods in `RuboCop::Cop::FormulaAudit::AssertStatements`.
+# Please instead update this file by running `bin/tapioca dsl RuboCop::Cop::FormulaAudit::AssertStatements`.
+
+class RuboCop::Cop::FormulaAudit::AssertStatements; end


Note that there are 21 rbi files that are useless that are introduced in this PR. It might be possible to make the dsl compiler logic smarter (I would need a class source API, like the method_source gem we rely on for methods) to avoid this. (The initial attempt used a hard-coded list to avoid this, which I also don't like.)

I think that tapioca should maybe not create rbi files when no action is taken in the decorator, so I might try filing an issue upstream first.

I think that tapioca should maybe not create rbi files when no action is taken in the decorator, so I might try filing an issue upstream first.

That's what I would expect it to do too.

dduugg · 2024-02-09T20:19:04Z

Library/Homebrew/sorbet/rbi/gems/[email protected]

@@ -1,319 +0,0 @@
-# typed: false


This is unrelated to the scope of the PR, but Tapioca ends up setting this to typed: false, bc it conflicts with the source rbi file in sorbet itself. We can just exclude it from generation (as we do with json for the same reason).

apainintheneck

Code looks good and it's great that we don't have to exclude and/or create by hand as many RBI files anymore. I left a bunch of nits but, of course, feel free to ignore them.

Library/Homebrew/sorbet/tapioca/compilers/rubocop.rb

Library/Homebrew/formula_cellar_checks.rb

apainintheneck · 2024-02-10T05:32:48Z

Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit/assert_statements.rbi

+# This is an autogenerated file for dynamic methods in `RuboCop::Cop::FormulaAudit::AssertStatements`.
+# Please instead update this file by running `bin/tapioca dsl RuboCop::Cop::FormulaAudit::AssertStatements`.
+
+class RuboCop::Cop::FormulaAudit::AssertStatements; end


I think that tapioca should maybe not create rbi files when no action is taken in the decorator, so I might try filing an issue upstream first.

That's what I would expect it to do too.

Library/Homebrew/sorbet/tapioca/compilers/rubocop.rb

apainintheneck · 2024-02-10T07:17:58Z

Library/Homebrew/sorbet/tapioca/compilers/rubocop.rb

+            if source.start_with?("def_node_matcher")
+              # https://github.com/Shopify/tapioca/blob/3341a9b/lib/tapioca/rbi_ext/model.rb#L89
+              klass.create_method(
+                method_name.to_s,
+                parameters:  [
+                  create_rest_param("node", type: "RuboCop::AST::Node"),
+                  create_kw_rest_param("kwargs", type: "T.untyped"),
+                  create_block_param("block", type: "T.untyped"),
+                ],
+                return_type: "T.untyped",
+              )
+            elsif source.start_with?("def_node_search")
+              klass.create_method(
+                method_name.to_s,
+                parameters:  [
+                  create_rest_param("node", type: "T.untyped"),
+                  create_block_param("block", type: "T.untyped"),
+                ],
+                return_type: method_name.to_s.end_with?("?") ? "T::Boolean" : "T.untyped",
+              )
+            end


Suggested change

if source.start_with?("def_node_matcher")

# https://github.com/Shopify/tapioca/blob/3341a9b/lib/tapioca/rbi_ext/model.rb#L89

klass.create_method(

method_name.to_s,

parameters: [

create_rest_param("node", type: "RuboCop::AST::Node"),

create_kw_rest_param("kwargs", type: "T.untyped"),

create_block_param("block", type: "T.untyped"),

],

return_type: "T.untyped",

)

elsif source.start_with?("def_node_search")

klass.create_method(

method_name.to_s,

parameters: [

create_rest_param("node", type: "T.untyped"),

create_block_param("block", type: "T.untyped"),

],

return_type: method_name.to_s.end_with?("?") ? "T::Boolean" : "T.untyped",

)

end

case source

when /^def_node_matcher/

# https://github.com/Shopify/tapioca/blob/3341a9b/lib/tapioca/rbi_ext/model.rb#L89

klass.create_method(

method_name.to_s,

parameters: [

create_rest_param("node", type: "RuboCop::AST::Node"),

create_kw_rest_param("kwargs", type: "T.untyped"),

create_block_param("block", type: "T.untyped"),

],

return_type: "T.untyped",

)

when /^def_node_search/

klass.create_method(

method_name.to_s,

parameters: [

create_rest_param("node", type: "T.untyped"),

create_block_param("block", type: "T.untyped"),

],

return_type: method_name.to_s.end_with?("?") ? "T::Boolean" : "T.untyped",

)

end

Nit: Use a case statement here for readability.

The version without regexes is more readable IMO, but sure 😛

Yeah, either way. It's personal preference I guess.

reitermarkus · 2024-02-10T15:52:14Z

Library/Homebrew/software_spec.rb

@@ -192,7 +192,7 @@ def depends_on(spec)
  def uses_from_macos(deps, bounds = {})
    if deps.is_a?(Hash)
      bounds = deps.dup
-      deps = [bounds.shift].to_h
+      deps = [T.unsafe(bounds).shift].to_h


Can we just add a sig here instead?

LMK if you have ideas, but i think it gets gnarly with type parameters and union types (and I think it would need to be in an rbi file due to the former). I think it's something I'd prefer to leave to a future PR (I'm thinking of doing a sweep over the T.unsafe call sites at some point anyway).

I think you can use multiple sigs.

sig { params(deps: String, bounds: T::Hash[String, T.any(Symbol, T::Array[Symbol])]).void } sig { params(deps: T::Hash[String, T.any(Symbol, T::Array[Symbol])]).void } def uses_from_macos(deps, bounds = {})

Actually, I think we don't even need the bounds parameter:

sig { params(deps: T.any(String, T::Hash[String, T.any(Symbol, T::Array[Symbol])])).void } def uses_from_macos(deps)

Overloading a method (by providing multiple signatures for the method) is only allowed for methods defined in the Ruby standard library.

Also, all params must be present in a sig.

(As a favor to the PR author, it would be appreciated to validate suggested changes before publishing them. Again, I also worry that there is excessive focus on this line, given the scope of this PR, and it may be better to refine in a future PR.)

As a favor to the PR author, it would be appreciated to validate suggested changes before publishing them.

I usually do, however I reviewed this on mobile.

Library/Homebrew/rubocops/cask/extend/node.rbi

Library/Homebrew/extend/ENV/shared.rb

dduugg · 2024-02-10T19:49:53Z

PTAL @apainintheneck @reitermarkus

reitermarkus

Looks good. Feel free to ignore uses_from_macos in favour of handling this in a future PR.

Bo98 · 2024-02-12T04:13:31Z

Library/Homebrew/sorbet/tapioca/compilers/rubocop.rb

+              klass.create_method(
+                method_name.to_s,
+                parameters:  [
+                  create_rest_param("node", type: "T.untyped"),


Is there a reason this node is untyped but def_node_matcher's isn't?

Thanks for the nudge here, I was able to tighten up the types in both branches in 439c8c18

MikeMcQuaid

Makes sense to me so far!

dduugg marked this pull request as draft February 5, 2024 06:10

dduugg changed the title ~~No hidden RBI~~ Remove use of hidden-definitions Feb 5, 2024

MikeMcQuaid approved these changes Feb 5, 2024

View reviewed changes

dduugg force-pushed the no-hidden-rbi branch 2 times, most recently from a7961a5 to 515b9bc Compare February 9, 2024 20:21

dduugg commented Feb 9, 2024

View reviewed changes

dduugg marked this pull request as ready for review February 9, 2024 20:29

dduugg force-pushed the no-hidden-rbi branch from 515b9bc to de332d8 Compare February 9, 2024 20:30

apainintheneck approved these changes Feb 10, 2024

View reviewed changes

reitermarkus reviewed Feb 10, 2024

View reviewed changes

Library/Homebrew/rubocops/cask/extend/node.rbi Show resolved Hide resolved

reitermarkus reviewed Feb 10, 2024

View reviewed changes

Library/Homebrew/rubocops/cask/extend/node.rbi Outdated Show resolved Hide resolved

reitermarkus requested changes Feb 10, 2024

View reviewed changes

Library/Homebrew/extend/ENV/shared.rb Outdated Show resolved Hide resolved

dduugg force-pushed the no-hidden-rbi branch from de332d8 to d122fdd Compare February 10, 2024 18:58

dduugg force-pushed the no-hidden-rbi branch from ed48801 to 9d7cdc0 Compare February 10, 2024 22:20

apainintheneck approved these changes Feb 10, 2024

View reviewed changes

dduugg force-pushed the no-hidden-rbi branch from 9d7cdc0 to 6e6f590 Compare February 10, 2024 22:28

reitermarkus approved these changes Feb 11, 2024

View reviewed changes

Bo98 reviewed Feb 12, 2024

View reviewed changes

dduugg added 10 commits February 11, 2024 20:27

wip

b2dc842

rm hidden.rbi

09f5a46

Add extend/node.rbi, remove unused methods

b010e39

Fix cask_help rbi

c83e017

Resolve violations from removing hidden.rbi

5b5cfe8

Add upstream rbi

a40fc0a

Remove hidden defs references

07fad78

Exclude msgpack from RBI generation

1dd0f6b

Make gather_constants dynamic

adfec16

Add rubocop exclusion

791ca27

dduugg added 5 commits February 11, 2024 20:27

Add Etc::Group struct members to upstream

772ddb0

brew tc --update-all

9dcde74

Add upstream JSON generator modules

1b84c5c

Code review fixes

aab0422

Tighten up the types

439c8c1

dduugg force-pushed the no-hidden-rbi branch from 6e6f590 to 439c8c1 Compare February 12, 2024 04:47

MikeMcQuaid approved these changes Feb 12, 2024

View reviewed changes

MikeMcQuaid merged commit 77e2765 into Homebrew:master Feb 12, 2024
26 checks passed

dduugg deleted the no-hidden-rbi branch February 12, 2024 14:55

github-actions bot added the outdated PR was locked due to age label Mar 14, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

dduugg changed the title ~~Remove use of hidden-definitions~~ Remove use of hidden-definitions in favor of Tapioca compilers Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove use of hidden-definitions in favor of Tapioca compilers #16586

Remove use of hidden-definitions in favor of Tapioca compilers #16586

dduugg commented Feb 5, 2024 •

edited

Loading

MikeMcQuaid left a comment

dduugg Feb 9, 2024

apainintheneck Feb 10, 2024

dduugg Feb 9, 2024

apainintheneck left a comment

apainintheneck Feb 10, 2024

apainintheneck Feb 10, 2024

dduugg Feb 10, 2024

apainintheneck Feb 10, 2024

reitermarkus Feb 10, 2024

dduugg Feb 10, 2024

reitermarkus Feb 11, 2024

reitermarkus Feb 11, 2024 •

edited

Loading

dduugg Feb 11, 2024

reitermarkus Feb 12, 2024

dduugg commented Feb 10, 2024

reitermarkus left a comment

Bo98 Feb 12, 2024

dduugg Feb 12, 2024

MikeMcQuaid left a comment

Remove use of hidden-definitions in favor of Tapioca compilers #16586

Remove use of hidden-definitions in favor of Tapioca compilers #16586

Conversation

dduugg commented Feb 5, 2024 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apainintheneck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Feb 10, 2024

reitermarkus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

dduugg commented Feb 5, 2024 •

edited

Loading

reitermarkus Feb 11, 2024 •

edited

Loading