[sorbet-runtime] Add T.must_because type assertion #6395

andrejewski-stripe · 2022-09-16T19:46:07Z

Adds a new type assertion method to sorbet-runtime, T.must_because(maybe_nil_value) {"reason_it_is_not_nil"}.

Motivation

The T.must error message is "Passed nil into T.must" which could be more descriptive/self-documenting in the cases where developers know (they usually should know) why they are assuring Sorbet that a value is not nil.

T.must_because is the exact same type narrowing but accepts a reason to document itself.

Test plan

See included automated tests.

jez · 2022-09-16T19:48:40Z

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

@@ -221,6 +221,34 @@ def self.must(arg)
    end
  end

+  # A convenience method to `raise` with a provided error reason when the argument


sorbet won’t automatically figure out that this method defined in sorbet-runtime exists. you’ll need to duplicate the signature to the file in rbi/ where T.must is defined. Also it’d be great if you added some tests in test/testdata/infer/must.rb to confirm that the RBI you’ve added works as expected

you likely also want to add a test in test/testdata/infer/must_untyped.rb to document that T.must and T.must_because have different behavior when called on some T.untyped value

If you want to go even further, the code which implements that error is in core/types/calls.cc. You could cargo cult some of that code to make that error apply to T.must_because as well if you like.

I bias towards have the same affordances so there's fewer gotchas and more reason to perhaps always use must_because over must

jez

^

jez · 2022-09-16T19:50:09Z

fast-follow those updates

fwiw, I think it should be fine to write the docs and link ahead of time, and fast-follow with a correction to the docs if the first attempt isn't right.

andrejewski-stripe · 2022-09-16T21:29:13Z

core/types/calls.cc

+                }
+                e.addErrorSection(args.args[0]->explainGot(gs, args.originForUninitialized));
+                auto replaceLoc = args.callLoc();
+                const auto locWithoutTMustBecause = args.callLoc().adjust(gs, 7, -1);


@jez What I'm assuming based on the numbers is:

- T.must(x) + x

But with T.must_because(x) {...} what could I do to properly rewrite this? Or should I axe this auto-correct for ease?

I have no idea why the original code was so complicated, and I probably wrote it.

I'm pretty sure you should be able to do

e.replaceWith("Remove `T.must_because`", replaceLoc, "{}", args.locs[0].source(gs).value());

(you might have to futz with it to get it to typecheck, not sure. but the idea is that args has something inside it that tells you what the location of the first argument to the method call is.

Looks like it might be called args.argLoc(0) now

jez · 2022-09-16T21:35:24Z

core/types/calls.cc

@@ -4107,6 +4141,7 @@ class T_Enum_tripleEq : public IntrinsicMethod {
 const vector<Intrinsic> intrinsics{
    {Symbols::T(), Intrinsic::Kind::Singleton, Names::untyped(), &T_untyped},
    {Symbols::T(), Intrinsic::Kind::Singleton, Names::must(), &T_must},
+    {Symbols::T(), Intrinsic::Kind::Singleton, Names::mustBecause(), &T_must_because},


You can likely get away with modifying T_must to be aware of whether it's handling T.must or T.must_because, and then use

Suggested change

{Symbols::T(), Intrinsic::Kind::Singleton, Names::mustBecause(), &T_must_because},

{Symbols::T(), Intrinsic::Kind::Singleton, Names::mustBecause(), &T_must},

which cuts down on some duplication if you're interested.

andrejewski-stripe · 2022-09-17T00:10:39Z

r? @jez I think I have everything passing now

jez · 2022-09-27T19:59:41Z

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

+  # to contain a non-nil value at this point.
+  #
+  # `sig {params(arg: T.nilable(A), reason_blk: T.proc.returns(String)).returns(A)}`
+  def self.must_because(arg, &reason_blk)


Can we remove the &reason_blk from this, for performance?

jez · 2022-09-27T20:00:56Z

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

+  # Intended to be used to promise sorbet that a given nilable value happens
+  # to contain a non-nil value at this point.
+  #
+  # `sig {params(arg: T.nilable(A), reason_blk: T.proc.returns(String)).returns(A)}`


Can you move this comment to the RBI, where it will be shown on hover? (The only people that will see it here are people who are changing sorbet-runtime.)

The current added comment in the RBI matches the comment structure of T.must in the RBI, it seems appropriate? I can remove the comment here in the runtime if it's not useful, I had copied the comment structure of the runtime T.must

jez

Changes look good, thanks. Few tweaks requested. I can merge after that.

andrejewski-stripe · 2022-09-27T21:49:13Z

rbi/sorbet/t.rbi

+  # argument is ever `nil`.
+  #
+  # For more, see https://sorbet.org/docs/type-assertions#tmust_because
+  sig {params(arg: T.untyped, reason_blk: T.proc.returns(String)).returns(T.untyped)}


@jez Now that we can implement T.must-likes with generics, should I be typing this signature more precisely: https://github.com/sorbet/sorbet/compare/andrejewski/t-must-because...andrejewski/t-must-because-stronger-rbi?expand=1

No, it's already computed by the intrinsic in calls.cc, so the signature is redundant.

Technically there are bugs in the signature-based approach, which are almost impossible to reproduce but for something like T.must I don't want to risk a bug in generics breaking it (the new generics features is nice for other people, but other code is lower stakes because it is going to be used vastly less frequently).

andrejewski-stripe · 2022-09-29T19:31:48Z

@jez All that's left is I'm not sure how to address this comment: #6395 (comment)

jez · 2022-10-18T19:31:55Z

I posted this on a Slack discussion thread before landing the change, but wanted to preserve the history in a comment, so here it is:

Thanks for sharing this for feedback! I like the motivation a lot and agree that it would be great to have custom error messages.

It does seem odd to me to have two methods that do the same thing and differ only in how they handle the error. Why not extend the existing T.must to allow for an optional extra parameter, e.g., T.must(maybe_nil_value, reason: "reason_it_is_not_nil")?

performance. that suggestion uses a keyword arg which is slow in hot paths. (there is a similar performance hit with an optional argument)

Tested on Ubuntu 20.04, Ruby 2.7.2

Time to run a no-op Ruby method call:

Vanilla Ruby method call: 17.98 ns

no optional args:

T.must(0): 29.562 ns
T.must(0) {'reason'}: 29.384 ns

optional positional arg:

T.must(0): 34.019 ns
T.must(0, 'reason'): 32.37 ns
T.must(0) {'reason'}: 35.735 ns

optional keyword arg:

T.must(0): 57.351 ns
T.must(0, reason: 'reason'): 57.538 ns
T.must(0) {'reason'}: 63.932 ns

Using yield + block_given? only in the raise case, no mention of &blk arg:

T.must(0): 29.138 ns
T.must(0) {'reason'}: 29.099 ns

Using explicit &blk arg + block_given? + yield:

T.must(0): 55.061 ns
T.must(0) {'reason'}: 57.372 ns

Even an optional positional arg is slower than an optional block. An optional keyword arg is almost 2x the runtime. Also mentioning the block arg explicitly is bad.

The Ruby VM is not a compiler. It has to do the work up front, and can’t reorder or defer work until it’s required.

That means that the act of putting &blk in the argument means that the Ruby VM is going to allocate a Proc object, regardless of whether you use any Proc methods.

Most of the time, the Ruby VM doesn’t actually allocate when given a block.

I tested this on Ruby 2.7, but it looks like it’s the same on Ruby 3.1. (I thought maybe the keyword arg changes would make the optional keyword arg case better but it doesn’t.)

Do you think given this analysis, we should devise a new syntax in Sorbet to annotate a yield without needing a &blk in the args? As described here: https://sorbet.org/docs/procs#annotating-methods-that-use-yield

yes, but not in the short term

but there shouldn't be a performance hit with passing an optional block to the existing T.must, right?

So:

foo = T.must(bar[0]) { "bar is expected to have at least one element" }

chris and I chatted, and we liked the way that T.must_because reads. Without a keyword arg label msg:, having the _because suffix makes it more clear that the block is computing an error message.

[sorbet-runtime] Add T.must_because type assertion

dfbff54

andrejewski-stripe requested a review from a team as a code owner September 16, 2022 19:46

andrejewski-stripe requested review from froydnj and removed request for a team September 16, 2022 19:46

jez reviewed Sep 16, 2022

View reviewed changes

jez requested changes Sep 16, 2022

View reviewed changes

jez assigned andrejewski-stripe Sep 16, 2022

froydnj removed their request for review September 16, 2022 19:52

andrejewski-stripe added 2 commits September 16, 2022 16:26

Add RBI, infer test data, and docs

7ceb165

Add untyped special casing

76a4b31

andrejewski-stripe commented Sep 16, 2022

View reviewed changes

jez reviewed Sep 16, 2022

View reviewed changes

andrejewski-stripe added 7 commits September 16, 2022 17:49

Fix auto-correct for T.must_because

520588a

Reuse T_must instrinsic

e06f296

Fix infer test

d888320

Add missing semicolon

9c8ed58

Make ternary into a one liner

e69829d

Ran the tests and formatter locally

316a9af

Try to fix infer test

3bc956a

andrejewski-stripe requested a review from jez September 19, 2022 17:37

jez reviewed Sep 27, 2022

View reviewed changes

jez approved these changes Sep 27, 2022

View reviewed changes

[perf] Remove explicit &reason_blk arg

f6143c4

andrejewski-stripe commented Sep 27, 2022

View reviewed changes

jez merged commit 2cb8794 into master Sep 29, 2022

jez deleted the andrejewski/t-must-because branch September 29, 2022 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sorbet-runtime] Add T.must_because type assertion #6395

[sorbet-runtime] Add T.must_because type assertion #6395

andrejewski-stripe commented Sep 16, 2022 •

edited

jez Sep 16, 2022

jez Sep 16, 2022

andrejewski-stripe Sep 16, 2022

jez left a comment

jez commented Sep 16, 2022

andrejewski-stripe Sep 16, 2022

jez Sep 16, 2022

jez Sep 16, 2022

jez Sep 16, 2022

andrejewski-stripe commented Sep 17, 2022

jez Sep 27, 2022

jez Sep 27, 2022

andrejewski-stripe Sep 27, 2022

jez left a comment

andrejewski-stripe Sep 27, 2022

jez Sep 28, 2022

jez Sep 28, 2022

andrejewski-stripe commented Sep 29, 2022

jez commented Oct 18, 2022

	{Symbols::T(), Intrinsic::Kind::Singleton, Names::mustBecause(), &T_must_because},
	{Symbols::T(), Intrinsic::Kind::Singleton, Names::mustBecause(), &T_must},

[sorbet-runtime] Add T.must_because type assertion #6395

[sorbet-runtime] Add T.must_because type assertion #6395

Conversation

andrejewski-stripe commented Sep 16, 2022 • edited

Motivation

Test plan

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

jez commented Sep 16, 2022

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

andrejewski-stripe commented Sep 17, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrejewski-stripe commented Sep 29, 2022

jez commented Oct 18, 2022

andrejewski-stripe commented Sep 16, 2022 •

edited