8000 Fix default_checked_level by peterzhu2118 · Pull Request #1173 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix default_checked_level #1173

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 2, 2019
Merged

Fix default_checked_level #1173

merged 2 commits into from
Jul 2, 2019

Conversation

peterzhu2118
Copy link
Contributor

Motivation

#1135 introduced T::Configuration.default_checked_level=, but if I do the following:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

  gem "sorbet-runtime"
end

require "sorbet-runtime"
T::Configuration.default_checked_level = :never

I get the error:

Traceback (most recent call last):
        2: from temp.rb:13:in `<main>'
        1: from /Users/peterzhu/.gem/ruby/2.5.5/gems/sorbet-runtime-0.4.4366/lib/types/configuration.rb:30:in `default_checked_level='
/Users/peterzhu/.gem/ruby/2.5.5/gems/sorbet-runtime-0.4.4366/lib/types/private/runtime_levels.rb:48:in `default_checked_level=': Set the default checked level earlier. There are already some methods whose sig blocks have evaluated which would not be affected by the new default. (RuntimeError)

This is caused by some typed methods in decorator.rb that is ran when the gem is required but doesn't have an explicit checked value so it calls T::Private::RuntimeLevels.default_checked_level before the line T::Configuration.default_checked_level = :never is ran which causes @has_read_default_checked_level to be set to true which causes the error (see code here).

Test plan

None

@peterzhu2118 peterzhu2118 requested a review from a team July 2, 2019 19:03
@ghost ghost requested review from aisamanra and removed request for a team July 2, 2019 19:03
jez
jez approved these changes Jul 2, 2019
View reviewed changes
Copy link
Collaborator
@jez jez left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Quick suggestion: i'd rather we use T::Sig::WithoutRuntime.sig for these cases. This makes it so that even if we read the default level earlier (i.e., right before running the block) the result would be the same.

@peterzhu2118 @jez
Apply suggestions from code review
9baa58b 
Co-Authored-By: Jake Zimmerman <zimmerman.jake@gmail.com>
@jez
Copy link
Collaborator
jez commented Jul 2, 2019

Awesome, i'll land this when the tests pass.

@jez jez self-assigned this Jul 2, 2019
@jez jez merged commit beecfbc into sorbet:master Jul 2, 2019
pje pushed a commit to pje/sorbet that referenced this pull request Jul 15, 2019
@peterzhu2118 @jez @pje
Fix default_checked_level (sorbet#1173)
50aafa2 
* Fix default_checked_level

* Apply suggestions from code review

Co-Authored-By: Jake Zimmerman <zimmerman.jake@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aisamanra aisamanra aisamanra approved these changes

@jez jez jez approved these changes

Assignees

@jez jez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peterzhu2118 @jez @aisamanra
0