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

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

Merged
merged 11 commits into from
Sep 29, 2022
1 change: 1 addition & 0 deletions core/tools/generate_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ NameDef names[] = {
{"syntheticBind", "<synthetic bind>"},
{"unsafe"},
{"must"},
{"mustBecause", "must_because"},
{"declareInterface", "interface!"},
{"declareAbstract", "abstract!"},
{"declareFinal", "final!"},
Expand Down
15 changes: 10 additions & 5 deletions core/types/calls.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1825,26 +1825,30 @@ class T_must : public IntrinsicMethod {
if (args.args.empty()) {
return;
}

auto methodName = args.name == Names::mustBecause() ? "T.must_because" : "T.must";

if (!args.args[0]->type.isFullyDefined()) {
if (auto e = gs.beginError(args.argLoc(0), errors::Infer::BareTypeUsage)) {
e.setHeader("`{}` applied to incomplete type `{}`", "T.must", args.args[0]->type.show(gs));
e.setHeader("`{}` applied to incomplete type `{}`", methodName, args.args[0]->type.show(gs));
}
return;
}
auto ret = Types::dropNil(gs, args.args[0]->type);
if (ret == args.args[0]->type) {
if (auto e = gs.beginError(args.argLoc(0), errors::Infer::InvalidCast)) {
if (args.args[0]->type.isUntyped()) {
e.setHeader("`{}` called on `{}`, which is redundant", "T.must", args.args[0]->type.show(gs));
e.setHeader("`{}` called on `{}`, which is redundant", methodName, args.args[0]->type.show(gs));
} else {
e.setHeader("`{}` called on `{}`, which is never `{}`", "T.must", args.args[0]->type.show(gs),
e.setHeader("`{}` called on `{}`, which is never `{}`", methodName, args.args[0]->type.show(gs),
"nil");
}
e.addErrorSection(args.args[0]->explainGot(gs, args.originForUninitialized));
auto replaceLoc = args.callLoc();
const auto locWithoutTMust = args.callLoc().adjust(gs, 7, -1);
const auto locWithoutTMust = args.argLoc(0);
if (replaceLoc.exists() && locWithoutTMust.exists()) {
e.replaceWith("Remove `T.must`", replaceLoc, "{}", locWithoutTMust.source(gs).value());
e.replaceWith(fmt::format("Remove `{}`", methodName), replaceLoc, "{}",
locWithoutTMust.source(gs).value());
}
}
}
Expand Down Expand Up @@ -4107,6 +4111,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},
{Symbols::T(), Intrinsic::Kind::Singleton, Names::all(), &T_all},
{Symbols::T(), Intrinsic::Kind::Singleton, Names::any(), &T_any},
{Symbols::T(), Intrinsic::Kind::Singleton, Names::nilable(), &T_nilable},
Expand Down
28 changes: 28 additions & 0 deletions gems/sorbet-runtime/lib/types/_types.rb
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,34 @@ def self.must(arg)
end
end

# A convenience method to `raise` with a provided error reason when the argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

# is `nil` and return it otherwise.
#
# Intended to be used as:
#
# needs_foo(T.must_because(maybe_gives_foo) {"reason_foo_should_not_be_nil"})
#
# Equivalent to:
#
# foo = maybe_gives_foo
# raise "reason_foo_should_not_be_nil" if foo.nil?
# needs_foo(foo)
#
# 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)}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

def self.must_because(arg, &reason_blk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return arg if arg
return arg if arg == false

begin
raise TypeError.new("Unexpected `nil` because #{yield}")
rescue TypeError => e # raise into rescue to ensure e.backtrace is populated
T::Configuration.inline_type_error_handler(e, {kind: 'T.must_because', value: arg, type: nil})
end
end

# A way to ask Sorbet to show what type it thinks an expression has.
# This can be useful for debugging and checking assumptions.
# In the runtime, merely returns the value passed in.
Expand Down
29 changes: 29 additions & 0 deletions gems/sorbet-runtime/test/types/must_because.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# frozen_string_literal: true
require_relative '../test_helper'

module Opus::Types::Test
class MustBecauseTest < Critic::Unit::UnitTest
EXAMPLE_REASON = 'some_must_because_reason'

it 'allows non-nil' do
assert_equal(:a, T.must_because(:a) {EXAMPLE_REASON})
assert_equal(0, T.must_because(0) {EXAMPLE_REASON})
assert_equal("", T.must_because("") {EXAMPLE_REASON})
assert_equal(false, T.must_because(false) {EXAMPLE_REASON})
end

it 'disallows nil' do
e = assert_raises(TypeError) do
T.must_because(nil) {EXAMPLE_REASON}
end

assert_equal("Unexpected `nil` because #{EXAMPLE_REASON}", e.message)
end

it 'does not calculate the reason unless nil is passed' do
T.must_because(:a) do
raise('reason block should not have been called')
end
end
end
end
10 changes: 10 additions & 0 deletions rbi/sorbet/t.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,16 @@ module T
sig {params(arg: T.untyped).returns(T.untyped)}
def self.must(arg); end

# Statically, declares to Sorbet that the argument is never `nil`, despite
# what the type system would otherwise infer for the type.
#
# At runtime, raises an exception contining the provided reason if the
# 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)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

def self.must_because(arg, &reason_blk); end

# A way to assert that a given branch of control flow is unreachable.
#
# Most commonly used to assert that a `case` or `if` expression exhaustively
Expand Down
17 changes: 17 additions & 0 deletions test/testdata/infer/must_because.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# typed: strict

def test_must_because # error: does not have a `sig`
x = T.cast(nil, T.nilable(String)) # error: `T.cast` is useless
T.assert_type!(T.must_because(x) {'reason'}, String)

T.must_because(x) {'reason'}
T.must_because()
# ^^ error: Not enough arguments
# ^ error: requires a block parameter

T.must_because(x) # error: requires a block parameter
T.must_because(x, 0)
# ^ error: Expected: `1`, got: `2`
# ^ error: requires a block parameter
T.must_because(x) {0} # error: Expected `String`
end
3 changes: 3 additions & 0 deletions test/testdata/infer/must_untyped.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,6 @@

T.must(T.unsafe(nil))
# ^^^^^^^^^^^^^ error: `T.must` called on `T.untyped`, which is redundant

T.must_because(T.unsafe(nil)) {'reason'}
# ^^^^^^^^^^^^^ error: `T.must_because` called on `T.untyped`, which is redundant
34 changes: 34 additions & 0 deletions website/docs/type-assertions.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,40 @@ end
→ View on sorbet.run
</a>

## `T.must_because`

`T.must_because`, like `T.must`, is for asserting a value of a
[nilable type](nilable-types.md) is not `nil`. It also takes a reason why the
value is not expected to be `nil`.

If the value is `nil` at runtime, the provided reason is included in the raised
exception's error message.

```rb
class A
extend T::Sig

sig {void}
def foo
y = T.must_because(nil) {'reason'}
puts y # error: This code is unreachable
end

sig {void}
def bar
vals = T.let([], T::Array[Integer])
x = vals.find {|a| a > 0}
T.reveal_type(x) # Revealed type: T.nilable(Integer)
y = T.must_because(x) {'reason'}
puts y # no static error
end
end
```

<a href="https://sorbet.run/#%23%20typed%3A%20true%0A%0Aclass%20A%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7Bvoid%7D%0A%20%20def%20foo%0A%20%20%20%20y%20%3D%20T.must_because%28nil%29%20%7B'reason'%7D%0A%20%20%20%20puts%20y%20%23%20error%3A%20This%20code%20is%20unreachable%0A%20%20end%0A%0A%20%20sig%20%7Bvoid%7D%0A%20%20def%20bar%0A%20%20%20%20vals%20%3D%20T.let%28%5B%5D%2C%20T%3A%3AArray%5BInteger%5D%29%0A%20%20%20%20x%20%3D%20vals.find%20%7B%7Ca%7C%20a%20%3E%200%7D%0A%20%20%20%20T.reveal_type%28x%29%20%23%20Revealed%20type%3A%20T.nilable%28Integer%29%0A%20%20%20%20y%20%3D%20T.must_because%28x%29%20%7B'reason'%7D%0A%20%20%20%20puts%20y%20%23%20no%20static%20error%0A%20%20end%0Aend">
→ View on sorbet.run
</a>

## `T.assert_type!`

`T.assert_type!` is similar to `T.let`: it is checked statically **and** at
Expand Down