Add type checking with Sorbet #273

mvidner · 2021-03-23T20:01:15Z

(Part of SUSE Hackweek: https://hackweek.suse.com/20/projects/type-check-yast-with-sorbet)

https://sorbet.org/ is a static (ahead of time) type checker.

How it works in general

Code is annotated with type declarations. Sorbet declarations are valid Ruby syntax and so can be inlined within the code or placed in separate files (RBI, for Ruby Interface).

The type checking is gradual, meaning that each file declares with a # typed: level header how strictly typed its code is: ignore is unchecked, false only checks constants (class names), true is the general level of checking (method names, argument counts, variable types) and there are strict and strong levels for even more checks.

The contents of the sorbet/ directory is autogenerated.

How it works in YaST

Manually written type declarations go to the rbi/ directory and are installed in /usr/share/YaST2/rbi to be used by dependent packages.

In yast2-ruby-bindings

This being a basic package used everywhere else, I suppose we will want very high level of type checking eventually.

As the first step, my goal was to add just enough types to enable detecting an (artificial) bug in yast2.rpm: Annotating Stage.rb (a very short file) just enough to detect that another file has a typo, Stage.inital instead of Stage.initial

to unignore debugger.rb

Sorbet cannot resolve constants off child classes

Explain to Sorbet that this module will be included in a regular Object, not a BasicObject https://sorbet.org/docs/error-reference#7003

then ran srb rbi suggest-typed srb rbi hidden-definitions

srb init is magic and that needs to be fixed its steps should be documented in `srb rbi` help anyway it deletes sorbet/rbi/sorbet-typed/lib/yast2/all/report.rbi and we will have to restore it

autocorrect https://srb.help/5046

They are Ruby Interface definitions for the Sorbet type checker https://sorbet.org/ They enable checking that users of yast2-ruby-bindings are calling us with the correct types.

mvidner · 2021-03-23T20:03:01Z

I've put this code to https://build.opensuse.org/package/show/home:mvidner:branches:YaST:Head/yast2-ruby-bindings

jreidinger · 2021-03-24T07:33:38Z

rbi/yast/builtins.rbi

@@ -0,0 +1,61 @@
+# typed: strong
+
+module Yast::Builtins


Hi interesting, code few questions: Is there check that verify that rbi and real rb code matches?

Good point!

If I make a mistake in builtins.rbi, it will go undetected because builtins.rb is still typed: false

convert.rb is typed: true but it defines its methods with metaprogramming so again, undetected :(

ops.rb is typed: false but if I change it to true there are few enough errors and one of them is in fact a mismatch that you ask about:

src/ruby/yast/ops.rb:113: Method Yast::Ops.get redefined without matching argument count. Expected: 3, got: 4 https://srb.help/4010 113 | def self.get(object, indexes, default = nil, skip_frames = 0) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ rbi/yast/ops.rbi:15: Previous definition 15 | def self.get(object, indexes, default=T.unsafe(nil), &block); end ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

jreidinger · 2021-03-24T07:33:53Z

rbi/yast/client.rbi

+
+module Yast
+  class Client
+    include I18n


is this needed? Cannot it load just from rb file?

Sorbet takes great care to be fast so I guess the major reason is to be able to read just the RBI files of your dependencies and not parse and analyze all their sources.

jreidinger · 2021-03-24T07:35:43Z

rbi/yast/i18n.rbi

+
+# Namespacing note:
+# if we say `module Yast::I18n` then String means ::String
+# if we said `module Yast; module I18n` then String would mean Yast::String


maybe it would be better to be explicit and just use ::String?

Yast::String should be destroyed, it is a design bug in YCP Killer.
Do you really want to write ::String instead of String everywhere?
Fortunately the two have such different APIs that an accidental confusion is resolved pretty quickly.

jreidinger · 2021-03-24T07:37:10Z

rbi/yast/scr.rbi

+      arg1: T.untyped
+    ).returns(T.untyped)
+  end
+  def self.Read(scr_path, arg1 = T.unsafe(nil)); end


Write is missing due to WIP or is there other reason?

Yup, just the minimal viable prototype. SCR.Write is pretty high in the TODO list.

jreidinger · 2021-03-24T07:38:09Z

rbi/yast/uishortcuts.rbi

@@ -0,0 +1,152 @@
+# typed: strong
+module Yast
+  module UIShortcuts


this looks quite fragile, maybe generate it by script? Also we basically kill type checking here as each type has own limitation for params

The way forward is to add more type information here, so generating this is a dead end.

So why did I include what appears to be a useless list?
It's because UIShortcuts is a mixin, and when I remove this RBI file, any typed: true class that includes it will fail on unknown methods. (seen in yast2.rpm)

jreidinger · 2021-03-24T07:39:06Z

rbi/yast/wfm.rbi

@@ -0,0 +1,24 @@
+# typed: strong
+module Yast::WFM
+  class NoParameter; end


this looks quite magic for me. For sure it will need some explanation

Hm, you are right, I did perpetrate magic here. Fortunately it did not have time to mutate so I can still remember the reason and will document it.

# See Args below class NoParameter ... # The following sig is a way to describe an optional parameter which has NOT a default value. # It is a union of # sig { params().returns(T::Array[YCPAny]) } # sig { params(i: Integer).returns(YCPAny) }

jreidinger · 2021-03-24T07:40:28Z

rbi/yast/wfm.rbi

+      Float,
+      Symbol,
+      String,
+      T::Array[T.untyped],


cannot we have recursion here like T::Array[YCPAny]

Let's try... nope.

jreidinger · 2021-03-24T07:43:32Z

tests/i18n_spec.rb

-  describe I18n do
+
+describe Yast do
+  describe Yast::I18n do


this looks string, I prefer just `describe Yast::I18n" without that Yast one.

s/string/strange/ - yes, I just wanted to avoid reindenting. will fix.

jreidinger · 2021-03-24T07:44:02Z

tests/i18n_spec.rb

+
+    let(:translated) { "translated".freeze }
+    let(:singular) { "untranslated".freeze }
+    let(:plural) { "plural".freeze }


why constants does not work?

Sorbet is stricter than Ruby and thinks that constants defined in one describe are not visible in a sibling describe. I will commit a shorter fix which just moves the constants up one block level.

yeap, that will look better

This reverts commit 7e13dc0.

Sorbet is stricter than Ruby and thinks that constants defined in one `describe` are not visible in a *sibling* `describe` This fix is simpler than the one reverted in the parent commit.

Side note about typechecking RSpec tests: It is supposed to work but my initial attempts errored at not recognizing the "context" method. I see typechecking RSpec as low priority because the tests themselves have 100% code coverage.

typed: true uncovers type error that will be fixed in following commits Splat arguments (*args) are poorly supported by sorbet and in cases where they cannot be factored away (like our pervasive logging API) the way around is to turn off the argument checks by making the receiver(!) untyped with T.unsafe

a ** b might overflow an Integer so use a literal number instead

This is an actual bug, even if it is hard to trigger - the spaceship operator <=> would have to go mad and return something else than -1 0 1. [SorbetFoundABug]

if FastGettext.text_domain raises then the ensure call to textdomain would use an uninitialized value (nil)

Found corner cases (which were not type correct YCP) that produced weird exceptions, made them explanatory TypeErrors. Removed deep_copy on an Integer. The sig lies a bit in that it allows FalseClass as the object argument. If I omit it, the static type checker will complain that the "else" clause after "when String, when Array" can never be reached. That would be correct if the sig were in the RB file, but it is in the RBI file, not used at run time. I need to figure out the proper way to have sigs in RB and RBI and check that they are in sync. Currently it does not work :(

mvidner and others added 21 commits June 20, 2019 10:07

srb init

d280d29

srb init again after adding fast_gettext

6baf16e

Added byebug and Yast.Report rbi

127f62a

to unignore debugger.rb

srb init again

e950819

Do not pollute the root namespace

2a1bb7f

Use let definitions instead of CONSTANTS

7e13dc0

Sorbet cannot resolve constants off child classes

Typed: ignore -> false

aa58e8f

include Kernel

c7b57ae

Explain to Sorbet that this module will be included in a regular Object, not a BasicObject https://sorbet.org/docs/error-reference#7003

Started explicit RBIs

205234b

then ran srb rbi suggest-typed srb rbi hidden-definitions

More RBI

2666bf2

RBI for Yast::Stage

5206086

More RBIs

606e3e3

Current gem versions

51b072a

srb init # because of new sorbet version

3777442

srb init is magic and that needs to be fixed its steps should be documented in `srb rbi` help anyway it deletes sorbet/rbi/sorbet-typed/lib/yast2/all/report.rbi and we will have to restore it

resolve Yast::Report in debugger

ce3b2af

typed: strong

c233d0b

autocorrect https://srb.help/5046

restored typed: strong

bf97fcd

Merge branch 'master' into sorbet

9856954

added a declaration to pass y2start_helpers.rb at typed: true

561be3c

Install RBIs

89088dd

They are Ruby Interface definitions for the Sorbet type checker https://sorbet.org/ They enable checking that users of yast2-ruby-bindings are calling us with the correct types.

version + changelog

fe1e260

mvidner mentioned this pull request Mar 24, 2021

Add type checking with Sorbet yast/yast-yast2#1148

Draft

jreidinger reviewed Mar 24, 2021

View reviewed changes

mvidner added 13 commits March 24, 2021 15:50

Revert "Use let definitions instead of CONSTANTS"

53592ea

This reverts commit 7e13dc0.

Move constants one block level up

322ea76

Sorbet is stricter than Ruby and thinks that constants defined in one `describe` are not visible in a *sibling* `describe` This fix is simpler than the one reverted in the parent commit.

Added a typed: false sigil to recently added files.

b84f508

Side note about typechecking RSpec tests: It is supposed to work but my initial attempts errored at not recognizing the "context" method. I see typechecking RSpec as low priority because the tests themselves have 100% code coverage.

Explain NoParameter

0695544

T.must says we're sure it's not nil

fe84eae

a ** b might overflow an Integer so use a literal number instead

Fix typo in variable name

374c18e

This is an actual bug, even if it is hard to trigger - the spaceship operator <=> would have to go mad and return something else than -1 0 1. [SorbetFoundABug]

Fix ensure logic if FastGettext.text_domain raises

04f65fb

if FastGettext.text_domain raises then the ensure call to textdomain would use an uninitialized value (nil)

Builtins.foreach (type only)

137b865

Builtins.foreach (nil untyped)

0d59919

Added more sigs, relaxed some to match behavior

38b00df

Add sorbet-runtime.gem dependency

6618b88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type checking with Sorbet #273

Add type checking with Sorbet #273

mvidner commented Mar 23, 2021

mvidner commented Mar 23, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

mvidner Mar 24, 2021

jreidinger Mar 24, 2021

Add type checking with Sorbet #273

Are you sure you want to change the base?

Add type checking with Sorbet #273

Conversation

mvidner commented Mar 23, 2021

How it works in general

How it works in YaST

In yast2-ruby-bindings

mvidner commented Mar 23, 2021

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

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