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
35 changes: 35 additions & 0 deletions core/types/calls.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1852,6 +1852,40 @@ class T_must : public IntrinsicMethod {
}
} T_must;

// T.must_because has handling equivalent to T.must for its first argument
class T_must_because : public IntrinsicMethod {
public:
void apply(const GlobalState &gs, const DispatchArgs &args, DispatchResult &res) const override {
if (args.args.empty()) {
return;
}
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_because", 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_because", args.args[0]->type.show(gs));
} else {
e.setHeader("`{}` called on `{}`, which is never `{}`", "T.must_because", args.args[0]->type.show(gs),
"nil");
}
e.addErrorSection(args.args[0]->explainGot(gs, args.originForUninitialized));
auto replaceLoc = args.callLoc();
const auto locWithoutTMustBecause = args.callLoc().adjust(gs, 7, -1);
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 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if (replaceLoc.exists() && locWithoutTMustBecause.exists()) {
e.replaceWith("Remove `T.must_because`", replaceLoc, "{}", locWithoutTMustBecause.source(gs).value());
}
}
}
res.returnType = move(ret);
}
} T_must_because;

class T_any : public IntrinsicMethod {
public:
void apply(const GlobalState &gs, const DispatchArgs &args, DispatchResult &res) const override {
Expand Down Expand Up @@ -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},
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

{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
13 changes: 13 additions & 0 deletions test/testdata/infer/must_because.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# 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
T.must_because(x) # error: requires a block parameter
T.must_because(x, 0)
# ^ error: Expected: `1`, got: `2`
T.must_because(x) {0} # error: Expected String
end
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