Remove enumerable runtime checking by default #3293

aisamanra · 2020-07-17T01:25:59Z

Motivation

This adds an extra parameter to valid? that defaults to false which governs whether it recurses into generic collection types for runtime type-checking. This allows us to avoid it by default (avoiding a big source of slowdown) but still do it if we need to, and opens up the possibility of future features which can "opt-in" to recursive checking for extra safety.

This also adds the deep parameter to assert_type!; methods which rely on "deep" runtime checking can retain their old externally-visible runtime behavior by using something like this:

sig {params(xs: T::Hash[String, T::Array[Integer]]).void}
def foo(xs)
  assert_type!(xs, T::Hash[String, T::Array[Integer]], deep: true)
  # ...
end

A standing question is whether we want to enable this by default for sigs using e.g. a construct like .checked(:deep). Also, should the deep parameter be called something like recursive?

Test plan

Updated tests to include both recursive and non-recursive variants of the same behavior. TODO: test over pay-server.

See included automated tests.

jez · 2020-07-20T19:42:51Z

gems/sorbet-runtime/lib/types/private/types/not_typed.rb

@@ -12,7 +12,7 @@ def name
  end

  # @override Base
-  def valid?(obj)
+  def valid?(obj, deep=false)


Why is this positional while the others are keyword? I generally prefer that optional boolean arguments are keyword arguments.

On second thought, seeing how this is used throughout the typed_enumerable code below, it seems really easy to forget to forward through the deep parameter. Can we make this parameter required, and change pay-server?

jez · 2020-07-20T19:46:37Z

gems/sorbet-runtime/lib/types/types/typed_array.rb

@@ -13,7 +13,7 @@ def underlying_class
    end

    # @override Base
-    def valid?(obj)
+    def valid?(obj, deep=false)
      obj.is_a?(Array) && super


ooc, we could short circuit all of these (typed array, typed hash, etc.) if deep is true, right? Why have you chosen to unconditionally call super?

jez · 2020-07-20T19:47:36Z

@aisamanra Can you remind me what the motivation is here?

aisamanra · 2020-07-20T22:38:53Z

This now has a recursively_valid? (instead of valid? with an argument) and I've removed the deep: keyword argument in favor of an entirely separate top-level helper method: this will make it trivial to delete if we ever want to do this.

jez · 2020-07-20T22:46:25Z

gems/sorbet-runtime/lib/types/_types.rb

+  # statically known and being checked appropriately. If `checked` is true, raises an exception at
+  # runtime if the value doesn't match the type.


This comment looks out of date?

Comment updated to be correct now. I've also added a bit to the docs, since I had forgotten to do that.

jez · 2020-07-20T23:01:21Z

gems/sorbet-runtime/lib/types/_types.rb

+  # in some cases this runtime check can be very expensive, especially
+  # with large collections of objects.


Suggested change

# in some cases this runtime check can be very expensive, especially

# with large collections of objects.

# in some cases this runtime check can be very expensive, especially

# with large collections of objects.

#

# This doesn't have any connection to static type refinements at all.

# If you additionally want the type to be cast statically, use `T.let` or `T.cast`.

jez · 2020-07-20T23:03:17Z

gems/sorbet-runtime/lib/types/types/typed_array.rb

@@ -13,10 +13,15 @@ def underlying_class
    end

    # @override Base
-    def valid?(obj)
+    def recursively_valid?(obj)
      obj.is_a?(Array) && super


Suggested change

obj.is_a?(Array) && super

valid?(obj) && super

^ This might be a change you want to make in a few other places too (just for consistency)

jez · 2020-07-20T23:04:41Z

gems/sorbet-runtime/lib/types/types/typed_enumerable.rb

      when Set
        obj.each do |item|
-          return false unless @type.valid?(item)
+          return false unless @type.recursively_valid?(item)


I like this a lot better with the recursively_valid? naming / boolean blindness issues.

jez · 2020-07-20T23:07:02Z

rbi/sorbet/t.rbi

@@ -32,6 +32,9 @@ module T
  sig {params(value: T.untyped, type: T.untyped, checked: T::Boolean).returns(BasicObject)}
  def self.assert_type!(value, type, checked: true); end

+  sig {params(value: T.untyped, type: T.untyped).returns(BasicObject)}
+  def self.check_type_recursive!(value, type); end


Do we want to put this in T::Private? Do we intend this to be a public API?

jez · 2020-07-20T23:08:14Z

gems/sorbet-runtime/test/types/props/decorator.rb

-
-      assert_raises(TypeError) do
-        StructHash.new(the_hash: {'foo' => {}})
-      end


Was this deleted just because it's identical to below? Is it worth keeping the .new call around and just deleting the assert_raises?

jez · 2020-07-20T23:09:05Z

gems/sorbet-runtime/test/types/casts.rb

+
+      it "does do recursive type-checking of arrays with `check_type_recursive!`" do
+        assert_raises(TypeError) do
+          T.check_type_recursive!([1], T::Array[String])


Yeah seeing this twice, I'm leaning towards at least putting this in T::Utils or something, and not putting it on T directly, because I think we don't really want to encourage people to depend on this accidentally.

I think that for now I'd advocate for T::Utils, but it depends on how heavily we think we'll remove this entirely in the future. I think it makes some sense to keep it around, but if we're otherwise pretty sure it'll be removed at some point, then maybe T::Private is the right place for it?

jez · 2020-07-20T23:11:46Z

gems/sorbet-runtime/lib/types/types/fixed_hash.rb

+      end
+
+      true
+    end


Feel free to push back here. Do you think this method could be improved by implementing it something like

valid?(obj) && <logic for recursively_valid>

?

(motivation: looks like a lot of duplication with valid?)

I was going to push back for performance concerns (since it'll re-traverse the hash twice, once for valid? and once for recursively_valid?) but on reflection, if you're using recursively_valid?, then you're already throwing perf out the window, so I'll do this refactor.

jez

Some small suggestions but I love the new overall direction.

djudd-stripe

This seems to make the ::Untyped variants of TypedArray, TypedHash etc mostly redundant. Maybe not for this PR, but should we follow up and remove them?

djudd-stripe · 2020-07-22T16:57:33Z

gems/sorbet-runtime/lib/types/types/fixed_array.rb

+    # @override Base
+    def recursively_valid?(obj)
+      obj.is_a?(Array) && obj.length == @types.length &&
+        obj.zip(@types).all? {|item, type| type.recursively_valid?(item)}


Might want to rebase after #3302 is merged

djudd-stripe · 2020-07-22T16:57:47Z

gems/sorbet-runtime/lib/types/types/fixed_hash.rb

@@ -16,6 +16,13 @@ def name
      "{#{@types.map {|(k, v)| "#{k}: #{v}"}.join(', ')}}"
    end

+    # @override Base
+    def recursively_valid?(obj)


Ditto, might want to rebase after #3302

djudd-stripe · 2020-07-22T16:58:54Z

gems/sorbet-runtime/lib/types/types/union.rb

@@ -42,6 +42,11 @@ def name
      end
    end

+    # @override Base
+    def recursively_valid?(obj)


Seems like the non-enumerable variants of recursively_valid? don't have any test coverage - am I missing something?

You're right—I've gone ahead and updated the types test now to test not only both code paths but ensure that the returned error messages are identical in every test where we'd expect them to be the same.

…t once

aisamanra · 2020-07-23T20:20:57Z

@jez I've made a few changes and so for thoroughness I figured I'd request a re-review. The changes since the last one are

setters and constructors to T::Struct and friends are now type-checked
the tests are now more thorough, and for everything where valid? and recursively_valid? are expected to be the same it'll test both and also test that the two parameters are identical.

jez · 2020-07-23T20:22:43Z

setters and constructors to T::Struct and friends are now type-checked

I assume this is at the request of kv-client, right? would you mind adding a note above the recursively_valid? calls mentioning something that it was an explicit decision to do that?

jez · 2020-07-23T20:30:23Z

gems/sorbet-runtime/test/types/types.rb

+      it 'fails if an element of the array is the wrong type under deep checking' do
+        type = T::Array[Integer]
+        value = [true]
+        msg = type.error_message_for_obj_recursive(value)


jez

Thanks!

djudd-stripe · 2020-07-23T20:46:05Z

gems/sorbet-runtime/lib/types/types/typed_range.rb

@@ -15,10 +15,15 @@ def name
    end

    # @override Base
-    def valid?(obj)
+    def recurisvely_valid?(obj)


Oof, yes. …apparently not only do we not test that, we must not use it anywhere in pay-server, either.

aisamanra force-pushed the gdritter/enumerable-runtime-take-two branch from fa57043 to 611e306 Compare July 20, 2020 19:21

aisamanra marked this pull request as ready for review July 20, 2020 19:21

aisamanra requested a review from a team as a code owner July 20, 2020 19:21

aisamanra requested review from jez and removed request for a team July 20, 2020 19:21

jez reviewed Jul 20, 2020

View reviewed changes

aisamanra requested a review from jez July 20, 2020 22:39

jez reviewed Jul 20, 2020

View reviewed changes

jez approved these changes Jul 20, 2020

View reviewed changes

djudd-stripe reviewed Jul 22, 2020

View reviewed changes

aisamanra added 7 commits July 23, 2020 10:52

Update tests to account for shallow/deep checking

6e17be7

Update TTypes RBI for new valid signature

bf2de42

One more RBI change

ea02479

A few more deep=false

1500c40

...one more, apparently

cc4fe44

One more

343081a

A few more

b317dce

aisamanra added 15 commits July 23, 2020 10:59

Split valid into valid and recursively-valid

0a05e5d

Update tests

51936c8

Update RBIs as well

db922b8

Remove recursive flag from assert_type! and add a new helper methods

332bb8f

Update assert_type! rbi

f29a395

Update comment

9200bfa

Add stuff to docs as well

4ec8140

Move check_type_recursive to T::Utils

b84f092

Re-add StructHash test

53707bb

Move check_type_recursive into private

3b30deb

Always used recursively_valid in prop setters

2f13c44

Always check recursively for struct fields and add a few more tests

18df751

Remove now-incorrect comment

5c940b9

Minimize diff from master

f2db389

Introduce helper method to check both recursive/nonrecursive checks a…

456ecd6

…t once

aisamanra force-pushed the gdritter/enumerable-runtime-take-two branch from af809ab to 456ecd6 Compare July 23, 2020 18:13

aisamanra requested a review from jez July 23, 2020 20:19

jez reviewed Jul 23, 2020

View reviewed changes

jez approved these changes Jul 23, 2020

View reviewed changes

djudd-stripe reviewed Jul 23, 2020

View reviewed changes

aisamanra added 2 commits July 23, 2020 13:48

Fix typo in TypedRange

22c05c7

Add comments about recursively_valid? in structs

506b31c

aisamanra merged commit aa3b1bb into master Jul 23, 2020

jez mentioned this pull request Aug 11, 2020

Better detection of enumerable type from an instance #3358

Merged

paracycle mentioned this pull request Aug 24, 2020

provide way to disable T.let / T.cast for production setups and debugging #3279

Open

aisamanra mentioned this pull request Aug 27, 2020

runtime checks on T::Array are incorrect (and do not match static checks) #3389

Closed

LandonSchropp mentioned this pull request Dec 15, 2020

Add option to turn on recursive type checking #3800

Open

jez deleted the gdritter/enumerable-runtime-take-two branch December 20, 2020 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove enumerable runtime checking by default #3293

Remove enumerable runtime checking by default #3293

aisamanra commented Jul 17, 2020 •

edited

Loading

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020 •

edited

Loading

jez commented Jul 20, 2020

aisamanra commented Jul 20, 2020

jez Jul 20, 2020

aisamanra Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

jez Jul 20, 2020

aisamanra Jul 21, 2020

jez Jul 20, 2020

aisamanra Jul 21, 2020

jez left a comment

djudd-stripe left a comment

djudd-stripe Jul 22, 2020

djudd-stripe Jul 22, 2020

djudd-stripe Jul 22, 2020

aisamanra Jul 23, 2020

aisamanra commented Jul 23, 2020

jez commented Jul 23, 2020

jez Jul 23, 2020

jez left a comment

djudd-stripe Jul 23, 2020

aisamanra Jul 23, 2020

		# statically known and being checked appropriately. If `checked` is true, raises an exception at
		# runtime if the value doesn't match the type.

		# in some cases this runtime check can be very expensive, especially
		# with large collections of objects.

+                    end
+                    true
+                  end

Remove enumerable runtime checking by default #3293

Remove enumerable runtime checking by default #3293

Conversation

aisamanra commented Jul 17, 2020 • edited Loading

Motivation

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jez Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

jez commented Jul 20, 2020

aisamanra commented Jul 20, 2020

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

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

jez left a comment

Choose a reason for hiding this comment

djudd-stripe 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

aisamanra commented Jul 23, 2020

jez commented Jul 23, 2020

Choose a reason for hiding this comment

jez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aisamanra commented Jul 17, 2020 •

edited

Loading

jez Jul 20, 2020 •

edited

Loading