Add type checking with Sorbet #1148
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this only works locally
Can be triggered by `sudo mv /usr/bin/systemctl{,.away}`
Test:
```sh
Y2DIR=library/systemd/src ruby -ryast -ryast2/systemd/target -e 'pp Yast2::Systemd::Target.get_default'
```
Before:
/usr/share/YaST2/lib/yast2/systemd/target.rb:83:in `get_default': uninitialized constant #<Class:Yast2::Systemd::Target>::SystemctlError (NameError)
After:
/home/martin/svn/yast-yast2/library/systemd/src/lib/yast2/systemd/target.rb:84:in `get_default': Systemctl command failed: #<OpenStruct exit=127, stderr="sh: /usr/bin/systemctl: No such file or directory\n", stdout="", command=" LANG=C TERM=dumb COLUMNS=1024 /usr/bin/systemctl --plain --full --no-legend --no-pager --no-ask-password get-default"> (Yast2::Systemctl::Error)
Found by sorbet
after SystemcltError bugfix and srb rbi todo to fix module/class mismatch
class A < described_class is too dynamic for sorbet.
declaring a constant in one block and using it in another is legal Ruby but confuses sorbet. move them a level up.
(git merge -s ours) At first I tried to do things the normal way: take the old 2019 sorbet branch and merge master into it. That meant merge conflicts EVERYWHERE because sorbet added "# typed" at the top while master deleted "encoding" at the top. Scratch that. In 2021 I re-did the earlier work on top of master and then formally merged the old branch to signify it does the same thing.
You may wonder why we have to do this because it is pretty obvious that a StageClass.new is of type StageClass. Well this inference works for a variable (stage = StageClass.new) but not for a Stage constant.
otherwise bundler will find a different bundler and stop
|
In https://github.com/yast/yast-yast2/compare/sorbet-typebug-demo?expand=1 I have added a bug (0e56df4) to show how a detected bug looks like. |
jreidinger
reviewed
Mar 24, 2021
| @@ -78,6 +80,8 @@ Requires: augeas-lenses | |||
| # For converting to/from punycode strings | |||
| Requires: sysconfig >= 0.80.0 | |||
| Requires: rubygem(%{rb_default_ruby_abi}:simpleidn) | |||
| # Type checking | |||
| BuildRequires: rubygem(%{rb_default_ruby_abi}:sorbet-runtime) | |||
There was a problem hiding this comment.
do you really want to do type checking during rpm build?
jreidinger
reviewed
Mar 24, 2021
| module Yast::ProductProfileClass::YaPI::SubscriptionTools; end | ||
| module Yast::Proxy; end | ||
| module Yast::SuSEFirewalldClass::FirewallCMDError; end | ||
| module Yast::XMLClass::Nokogiri; end |
There was a problem hiding this comment.
this looks wrong as nokogiri should be autogenerated, not?
with new sigs in yast2-ruby-bindings these files can be typechecked after adding sigs Builtins: foreach regexpmatch, WFM::CallFunction
IMHO this is a design bug in Ruby copied into Sorbet. A Boolean class should exist in Ruby, or Sorbet should infer it, without us having to declare it with T.let Without this: ``` ./library/packages/src/modules/PackageKit.rb:52: Changing the type of a variable in a loop is not permitted https://srb.help/7001 52 | ret = true if Builtins.regexpmatch(line, "boolean.*true") ^^^^ Existing variable has type: FalseClass Attempting to change type to: TrueClass ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Part of SUSE Hackweek: https://hackweek.suse.com/20/projects/type-check-yast-with-sorbet)
See yast/yast-ruby-bindings#273 for context