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

Make runtime type check attr_accessor writers #6426

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 3 commits into from
Oct 12, 2022
Merged

Make runtime type check attr_accessor writers #6426

merged 3 commits into from
Oct 12, 2022

Conversation

paracycle
Copy link
Collaborator
@paracycle paracycle commented Sep 23, 2022 •
edited
Loading

Sorbet runtime has never checked the writer for attr_accessor defined methods since that is the only case where a single signature needs to be matched up to two methods.

This PR adds an exception for the reader method of an attr_accessor triggered method added handling. For those cases, after we clear the current declaration, we create a new writer declaration with the correct params and make it the current declaration. That makes sure that the next method added handling, which should be for the writer method, can find and process a correct declaration.

Credit to @XrXr for giving me the original idea to check the caller_location label to see if we are in an attr_accessor call or not.

Motivation

Fixes: #5685

Test plan

Extended existing automated tests, and removed a TODO.

@paracycle paracycle requested a review from a team as a code owner September 23, 2022 21:42
@paracycle paracycle requested review from froydnj and removed request for a team September 23, 2022 21:42
@jez jez requested review from jez and removed request for froydnj September 23, 2022 21:44
paracycle
paracycle commented Sep 23, 2022
View reviewed changes
paracycle
paracycle commented Sep 23, 2022
View reviewed changes
@paracycle paracycle force-pushed the uk-make-attr_accessor-work branch 3 times, most recently from 582988d to ded6761 Compare September 23, 2022 23:09
jez
jez reviewed Sep 26, 2022
View reviewed changes
jez
jez reviewed Sep 26, 2022
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.

This looks neat, thanks! I'll have to test it on Stripe's codebase to make sure. I'd prefer to do that after the implementation is adjusted to use the alternate current_declaration strategy you mentioned.

@paracycle paracycle force-pushed the uk-make-attr_accessor-work branch from ded6761 to 78b052a Compare September 28, 2022 14:09
@paracycle
Copy link
Collaborator Author

This looks neat, thanks!

Thank you, happy to finally get this working. Also forgot to thank @XrXr for giving me the original idea to check the caller_location label to see if we are in an attr_accessor call or not.

I'll have to test it on Stripe's codebase to make sure. I'd prefer to do that after the implementation is adjusted to use the alternate current_declaration strategy you mentioned.

I think this is ready to run that check, though I suspect this might turn up some unexpected cases at runtime since attr_accessor writers haven't been tested properly until now.

@paracycle paracycle force-pushed the uk-make-attr_accessor-work branch from 78b052a to 5747dcf Compare September 28, 2022 17:10
@jez jez self-requested a review September 29, 2022 18:04
@paracycle
Copy link
Collaborator Author

@jez I am curious about what this is going to unearth in the Stripe codebase. Did you have any time to run it at all?

@jez
Copy link
Collaborator
jez commented Oct 4, 2022

Sorry I haven't had a chance to test it.

My guess is that it's going to expose a bunch of people either passing in mocked values or nil values in tests that weren't being enforced prior.

I should have some time to test this today though, so I will get back to you.

@jez
Copy link
Collaborator
jez commented Oct 4, 2022

http://go/builds/bui_MYQXn9olG8SXje

@jez
Copy link
Collaborator
jez commented Oct 4, 2022

The first build failed with thousands of test failures 😢

I will take some time to try to figure out whether they all share a couple common causes, or what might be required to fix them all.

@jez
Copy link
Collaborator
jez commented Oct 11, 2022

Heads up Ufuk, I'm going to hijack your PR to leave some documentation for posterity in the next comment. (I just want to get a permalink that I can paste into some comments in Stripe's codebase—the comment itself is not addressed to you.)

@jez
Copy link
Collaborator
jez commented Oct 11, 2022

👋 Hey there! If you're reading this, it likely means that you're looking at some attr_reader/attr_writer code and wondering why it's not using attr_accessor.

The PR whose comments you're reading right now fixes a bug in Sorbet where Sorbet previously did not wrap the writer method defined by attr_accessor in runtime type checking. When this bug was fixed, it exposed some bugs in the codebase where despite having a sig, the code was not actually conforming to the sig at runtime (sometimes this is because the code was using mocks in tests; sometimes this is because the code wasn't properly handling nil; etc. There are many reasons).

To allow the Sorbet version to be upgraded, instead of fixing the root causes, we have simply rewritten the attr_accessor's into separate attr_reader and attr_writer calls. This preserves the runtime typing behavior that used to exist, making it easier to upgrade to a new Sorbet version (and lock in the bugfix for all new code).

If you are seeing this in your code, feel free to either add a sig to the attr_writer, or merge the attr_reader and attr_writer back into one attr_accessor call, and fix any ensuing type errors and test failures. Your teammates will thank you!

If you have questions, feel free to reach out.
< 8000 div data-view-component="true" class="comment-reactions js-reactions-container js-reaction-buttons-container social-reactions reactions-container d-none">

@jez
Copy link
Collaborator
jez commented Oct 12, 2022

The first build failed with thousands of test failures 😢

I will take some time to try to figure out whether they all share a couple common causes, or what might be required to fix them all.

It turns out that all the test failures blamed back to 5 individual attr_accessor calls.

@jez
Copy link
Collaborator
jez commented Oct 12, 2022

Thanks! This one has been broken for a long time, great to see it fixed.

@jez jez merged commit b1a7f2c into sorbet:master Oct 12, 2022
@Qqwy
Copy link
Qqwy commented Oct 12, 2022

Thank you very much for fixing this issue! ❤️

@paracycle paracycle deleted the uk-make-attr_accessor-work branch October 12, 2022 09:24
@andyw8 andyw8 mentioned this pull request Oct 12, 2022
Closed
jez added a commit that referenced this pull request Oct 24, 2022
@jez
Revert "Make runtime type check attr_accessor writers (#6426)"
bdecc7e 
This reverts commit b1a7f2c.
jez added a commit that referenced this pull request Oct 24, 2022
@jez
Revert "Make runtime type check attr_accessor writers (#6426)" (#6505)
d53ffb3 
This reverts commit b1a7f2c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jez jez jez 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.

Signatures for attr_accessor won't add runtime checks to the writer method
3 participants
@paracycle @jez @Qqwy
0