8000 Revert "Make runtime type check `attr_accessor` writers (#6426)" by jez · Pull Request #6505 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Revert "Make runtime type check attr_accessor writers (#6426)" #6505

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 1 commit into from
Oct 24, 2022
Merged

Revert "Make runtime type check attr_accessor writers (#6426)" #6505

merged 1 commit into from
Oct 24, 2022

Conversation

jez
Copy link
Collaborator
@jez jez commented Oct 24, 2022

This reverts commit b1a7f2c.

Motivation

There are two causes. Cause 1, discussed on Slack:

Ufuk Kayserilioglu I’m having some problems landing your attr_accessor change on Stripe’s codebase. In my initial testing, I thought there were no problems, and then when I went to deploy certain services, I saw crashes at code load time.

It turns out that if there is another gem or another part of the codebase that adds a method_added hook, it shows up in the backtrace, which throws off the “the attr_accessor call will be exactly 3 frames above us” computation

vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10483/lib/types/private/methods/_methods.rb:216:in `attr_accessor_reader?'
vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10483/lib/types/private/methods/_methods.rb:265:in `_on_method_added'
vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10483/lib/types/private/methods/_methods.rb:554:in `method_added'
vendor/bundle/ruby/2.7.0/gems/blankslate-2.1.2.4/lib/blankslate.rb:80:in `method_added'
manage/framework/erb_context.rb:12:in `attr_accessor'
manage/framework/erb_context.rb:12:in `<class:ErbContext>'
manage/framework/erb_context.rb:8:in `<module:Framework>'
manage/framework/erb_context.rb:7:in `<compiled>'
primus/lib/require.rb:854:in `require'

not sure what the best way is to fix this is going to be. I can work around it for the time being, but I would love if you could lend some eyes as well.

Cause 2:

i tried again with the upgrade (after fixing the override problems) and we’re seeing substantially increased memory, causing problems in our CI environment.

It’s unclear but my guess is that this is a side effect of adding sig’s to thousands of attr_accessor calls.

There are some other changes that we need to make to sorbet-runtime, that this attr_accessor change is blocking. While we work on a change to make it work in the presence of weird call stack depths and to give me time to figure out how to deal with the memory increase, I’m going to revert the attr_accessor PR to make way for the other changes that need to be made. We can re-measure the memory problems before landing whatever fixes we would make to the call stack depth problem.

Test plan

n/a

@jez jez requested a review from a team as a code owner October 24, 2022 20:00
@jez jez requested review from froydnj and removed request for a team October 24, 2022 20:00
@jez jez enabled auto-merge (squash) October 24, 2022 20:06
@jez jez merged commit d53ffb3 into master Oct 24, 2022
@jez jez deleted the jez-revert branch October 24, 2022 20:16
@vinistock vinistock mentioned this pull request Oct 25, 2022
Merged
dirceu added a commit to Shopify/tapioca that referenced this pull request Oct 25, 2022
@dirceu
Revert the change about attr_* runtime checks
875eae8 
Reverts part of #1227 while
sorbet/sorbet#6505 is not added back.
@paracycle paracycle mentioned this pull request Mar 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@froydnj froydnj froydnj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jez @froydnj
0